lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Message-ID: <98efc508-c431-2509-5799-96decc124136@omp.ru> Date: Sat, 23 Dec 2023 22:39:50 +0300 From: Sergey Shtylyov <s.shtylyov@....ru> To: Claudiu <claudiu.beznea@...on.dev>, <davem@...emloft.net>, <edumazet@...gle.com>, <kuba@...nel.org>, <pabeni@...hat.com>, <yoshihiro.shimoda.uh@...esas.com>, <wsa+renesas@...g-engineering.com>, <mitsuhiro.kimura.kc@...esas.com> CC: <netdev@...r.kernel.org>, <linux-renesas-soc@...r.kernel.org>, <linux-kernel@...r.kernel.org>, Claudiu Beznea <claudiu.beznea.uj@...renesas.com> Subject: Re: [PATCH net v2 1/1] net: ravb: Wait for operation mode to be applied On 12/22/23 2:35 PM, Claudiu wrote: > From: Claudiu Beznea <claudiu.beznea.uj@...renesas.com> > > CSR.OPS bits specify the current operating mode and (according to > documentation) they are updated by HW when the operating mode change > request is processed. To comply with this check CSR.OPS before proceeding. > > Commit introduces ravb_set_opmode() that does all the necessities for > setting the operating mode (set DMA.CCC and wait for CSR.OPS) and call it > where needed. This should comply with all the HW manuals requirements as > different manual variants specify that different modes need to be checked > in CSR.OPS when setting DMA.CCC. > > Fixes: c156633f1353 ("Renesas Ethernet AVB driver proper") > Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@...renesas.com> > --- > drivers/net/ethernet/renesas/ravb_main.c | 52 ++++++++++++++---------- > 1 file changed, 31 insertions(+), 21 deletions(-) > > diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c > index 664eda4b5a11..ae99d035a3b6 100644 > --- a/drivers/net/ethernet/renesas/ravb_main.c > +++ b/drivers/net/ethernet/renesas/ravb_main.c > @@ -66,14 +66,15 @@ int ravb_wait(struct net_device *ndev, enum ravb_reg reg, u32 mask, u32 value) > return -ETIMEDOUT; > } > > -static int ravb_config(struct net_device *ndev) > +static int ravb_set_opmode(struct net_device *ndev, u32 opmode) Since you pass the complete CCC register value below, you should rather call the function ravb_set_ccc() and call the parameter opmode ccc. > { > + u32 csr_opmode = 1UL << opmode; Please use the correct expression, 1U << (ccc & CCC_OPC) instead. And I'd suggest calling the variable csr_ops or just ops. > int error; > > - /* Set config mode */ > - ravb_modify(ndev, CCC, CCC_OPC, CCC_OPC_CONFIG); > - /* Check if the operating mode is changed to the config mode */ > - error = ravb_wait(ndev, CSR, CSR_OPS, CSR_OPS_CONFIG); > + /* Set operating mode */ > + ravb_modify(ndev, CCC, CCC_OPC, opmode); > + /* Check if the operating mode is changed to the requested one */ > + error = ravb_wait(ndev, CSR, CSR_OPS, csr_opmode); > if (error) > netdev_err(ndev, "failed to switch device to config mode\n"); s/config/requested/? Or just print out that mode... [...] > @@ -2560,21 +2559,23 @@ static int ravb_set_gti(struct net_device *ndev) > return 0; > } > > -static void ravb_set_config_mode(struct net_device *ndev) > +static int ravb_set_config_mode(struct net_device *ndev) > { > struct ravb_private *priv = netdev_priv(ndev); > const struct ravb_hw_info *info = priv->info; > + int error; > > if (info->gptp) { > - ravb_modify(ndev, CCC, CCC_OPC, CCC_OPC_CONFIG); > + error = ravb_set_opmode(ndev, CCC_OPC_CONFIG); Don't we need to return on error here? > /* Set CSEL value */ > ravb_modify(ndev, CCC, CCC_CSEL, CCC_CSEL_HPB); > } else if (info->ccc_gac) { > - ravb_modify(ndev, CCC, CCC_OPC, CCC_OPC_CONFIG | > - CCC_GAC | CCC_CSEL_HPB); > + error = ravb_set_opmode(ndev, CCC_OPC_CONFIG | CCC_GAC | CCC_CSEL_HPB); See, you pass more than just CCC.OPC value here -- need to mask it out above... [...] > @@ -2917,8 +2921,9 @@ static void ravb_remove(struct platform_device *pdev) > dma_free_coherent(ndev->dev.parent, priv->desc_bat_size, priv->desc_bat, > priv->desc_bat_dma); > > - /* Set reset mode */ > - ravb_write(ndev, CCC_OPC_RESET, CCC); > + error = ravb_set_opmode(ndev, CCC_OPC_RESET); > + if (error) > + netdev_err(ndev, "Failed to reset ndev\n"); ravb_set_opmode() will have complained already at this point... [...] MBR, Sergey
Powered by blists - more mailing lists