| Message ID | 20241018194244.280322-1-marex@denx.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | wifi: wilc1000: Add proper error handling for remaining CMD52 | expand |
Hi Marek, On 10/18/24 21:41, Marek Vasut wrote: > A few of the CMD52 calls did not have any error handling, add it. > This prevents odd errors like "Unexpected interrupt (1) int=nnn" > when the CMD52 fails just above in the IRQ handler and the CMD52 > error code is ignored by the driver. Fill the error handling in. > Sort the variables in those affected functions while at it. Note > that the error code itself is already printed in wilc_sdio_cmd52(). > > Signed-off-by: Marek Vasut <marex@denx.de> > --- > Cc: "David S. Miller" <davem@davemloft.net> > Cc: Adham Abozaeid <adham.abozaeid@microchip.com> > Cc: Ajay Singh <ajay.kathat@microchip.com> > Cc: Alexis Lothoré <alexis.lothore@bootlin.com> > Cc: Claudiu Beznea <claudiu.beznea@tuxon.dev> > Cc: Conor Dooley <conor+dt@kernel.org> > Cc: Eric Dumazet <edumazet@google.com> > Cc: Jakub Kicinski <kuba@kernel.org> > Cc: Kalle Valo <kvalo@kernel.org> > Cc: Krzysztof Kozlowski <krzk+dt@kernel.org> > Cc: Paolo Abeni <pabeni@redhat.com> > Cc: Rob Herring <robh@kernel.org> > Cc: devicetree@vger.kernel.org > Cc: linux-wireless@vger.kernel.org > Cc: netdev@vger.kernel.org > --- > .../net/wireless/microchip/wilc1000/sdio.c | 27 ++++++++++++++----- > 1 file changed, 21 insertions(+), 6 deletions(-) > > diff --git a/drivers/net/wireless/microchip/wilc1000/sdio.c b/drivers/net/wireless/microchip/wilc1000/sdio.c > index 5262c8846c13d..170470d1c2092 100644 > --- a/drivers/net/wireless/microchip/wilc1000/sdio.c > +++ b/drivers/net/wireless/microchip/wilc1000/sdio.c > @@ -769,8 +769,10 @@ static int wilc_sdio_init(struct wilc *wilc, bool resume) > > static int wilc_sdio_read_size(struct wilc *wilc, u32 *size) > { > - u32 tmp; > + struct sdio_func *func = dev_to_sdio_func(wilc->dev); > struct sdio_cmd52 cmd; > + u32 tmp; > + int ret; > > /** > * Read DMA count in words > @@ -780,12 +782,20 @@ static int wilc_sdio_read_size(struct wilc *wilc, u32 *size) > cmd.raw = 0; > cmd.address = WILC_SDIO_INTERRUPT_DATA_SZ_REG; > cmd.data = 0; > - wilc_sdio_cmd52(wilc, &cmd); > + ret = wilc_sdio_cmd52(wilc, &cmd); > + if (ret) { > + dev_err(&func->dev, "Fail cmd 52, set DATA_SZ[0] register...\n"); I don't get the log message, why "set" DATA_SZ[0] ? This helper is rather trying to read it. Same for the other logs added below > + return ret; > + } > tmp = cmd.data; > > cmd.address = WILC_SDIO_INTERRUPT_DATA_SZ_REG + 1; > cmd.data = 0; > - wilc_sdio_cmd52(wilc, &cmd); > + ret = wilc_sdio_cmd52(wilc, &cmd); > + if (ret) { > + dev_err(&func->dev, "Fail cmd 52, set DATA_SZ[1] register...\n"); > + return ret; > + } > tmp |= (cmd.data << 8); > > *size = tmp; > @@ -796,9 +806,10 @@ static int wilc_sdio_read_int(struct wilc *wilc, u32 *int_status) > { > struct sdio_func *func = dev_to_sdio_func(wilc->dev); > struct wilc_sdio *sdio_priv = wilc->bus_data; > - u32 tmp; > - u8 irq_flags; > struct sdio_cmd52 cmd; > + u8 irq_flags; > + u32 tmp; > + int ret; > > wilc_sdio_read_size(wilc, &tmp); > > @@ -817,7 +828,11 @@ static int wilc_sdio_read_int(struct wilc *wilc, u32 *int_status) > cmd.raw = 0; > cmd.read_write = 0; > cmd.data = 0; > - wilc_sdio_cmd52(wilc, &cmd); > + ret = wilc_sdio_cmd52(wilc, &cmd); > + if (ret) { > + dev_err(&func->dev, "Fail cmd 52, set IRQ_FLAG register...\n"); > + return ret; > + } > irq_flags = cmd.data; > > if (sdio_priv->irq_gpio)
On 10/22/24 11:11 AM, Alexis Lothoré wrote: Hi, >> diff --git a/drivers/net/wireless/microchip/wilc1000/sdio.c b/drivers/net/wireless/microchip/wilc1000/sdio.c >> index 5262c8846c13d..170470d1c2092 100644 >> --- a/drivers/net/wireless/microchip/wilc1000/sdio.c >> +++ b/drivers/net/wireless/microchip/wilc1000/sdio.c >> @@ -769,8 +769,10 @@ static int wilc_sdio_init(struct wilc *wilc, bool resume) >> >> static int wilc_sdio_read_size(struct wilc *wilc, u32 *size) >> { >> - u32 tmp; >> + struct sdio_func *func = dev_to_sdio_func(wilc->dev); >> struct sdio_cmd52 cmd; >> + u32 tmp; >> + int ret; >> >> /** >> * Read DMA count in words >> @@ -780,12 +782,20 @@ static int wilc_sdio_read_size(struct wilc *wilc, u32 *size) >> cmd.raw = 0; >> cmd.address = WILC_SDIO_INTERRUPT_DATA_SZ_REG; >> cmd.data = 0; >> - wilc_sdio_cmd52(wilc, &cmd); >> + ret = wilc_sdio_cmd52(wilc, &cmd); >> + if (ret) { >> + dev_err(&func->dev, "Fail cmd 52, set DATA_SZ[0] register...\n"); > > I don't get the log message, why "set" DATA_SZ[0] ? This helper is rather trying > to read it. Same for the other logs added below Fixed in V2 , s@set@get@ , thanks !
diff --git a/drivers/net/wireless/microchip/wilc1000/sdio.c b/drivers/net/wireless/microchip/wilc1000/sdio.c index 5262c8846c13d..170470d1c2092 100644 --- a/drivers/net/wireless/microchip/wilc1000/sdio.c +++ b/drivers/net/wireless/microchip/wilc1000/sdio.c @@ -769,8 +769,10 @@ static int wilc_sdio_init(struct wilc *wilc, bool resume) static int wilc_sdio_read_size(struct wilc *wilc, u32 *size) { - u32 tmp; + struct sdio_func *func = dev_to_sdio_func(wilc->dev); struct sdio_cmd52 cmd; + u32 tmp; + int ret; /** * Read DMA count in words @@ -780,12 +782,20 @@ static int wilc_sdio_read_size(struct wilc *wilc, u32 *size) cmd.raw = 0; cmd.address = WILC_SDIO_INTERRUPT_DATA_SZ_REG; cmd.data = 0; - wilc_sdio_cmd52(wilc, &cmd); + ret = wilc_sdio_cmd52(wilc, &cmd); + if (ret) { + dev_err(&func->dev, "Fail cmd 52, set DATA_SZ[0] register...\n"); + return ret; + } tmp = cmd.data; cmd.address = WILC_SDIO_INTERRUPT_DATA_SZ_REG + 1; cmd.data = 0; - wilc_sdio_cmd52(wilc, &cmd); + ret = wilc_sdio_cmd52(wilc, &cmd); + if (ret) { + dev_err(&func->dev, "Fail cmd 52, set DATA_SZ[1] register...\n"); + return ret; + } tmp |= (cmd.data << 8); *size = tmp; @@ -796,9 +806,10 @@ static int wilc_sdio_read_int(struct wilc *wilc, u32 *int_status) { struct sdio_func *func = dev_to_sdio_func(wilc->dev); struct wilc_sdio *sdio_priv = wilc->bus_data; - u32 tmp; - u8 irq_flags; struct sdio_cmd52 cmd; + u8 irq_flags; + u32 tmp; + int ret; wilc_sdio_read_size(wilc, &tmp); @@ -817,7 +828,11 @@ static int wilc_sdio_read_int(struct wilc *wilc, u32 *int_status) cmd.raw = 0; cmd.read_write = 0; cmd.data = 0; - wilc_sdio_cmd52(wilc, &cmd); + ret = wilc_sdio_cmd52(wilc, &cmd); + if (ret) { + dev_err(&func->dev, "Fail cmd 52, set IRQ_FLAG register...\n"); + return ret; + } irq_flags = cmd.data; if (sdio_priv->irq_gpio)
A few of the CMD52 calls did not have any error handling, add it. This prevents odd errors like "Unexpected interrupt (1) int=nnn" when the CMD52 fails just above in the IRQ handler and the CMD52 error code is ignored by the driver. Fill the error handling in. Sort the variables in those affected functions while at it. Note that the error code itself is already printed in wilc_sdio_cmd52(). Signed-off-by: Marek Vasut <marex@denx.de> --- Cc: "David S. Miller" <davem@davemloft.net> Cc: Adham Abozaeid <adham.abozaeid@microchip.com> Cc: Ajay Singh <ajay.kathat@microchip.com> Cc: Alexis Lothoré <alexis.lothore@bootlin.com> Cc: Claudiu Beznea <claudiu.beznea@tuxon.dev> Cc: Conor Dooley <conor+dt@kernel.org> Cc: Eric Dumazet <edumazet@google.com> Cc: Jakub Kicinski <kuba@kernel.org> Cc: Kalle Valo <kvalo@kernel.org> Cc: Krzysztof Kozlowski <krzk+dt@kernel.org> Cc: Paolo Abeni <pabeni@redhat.com> Cc: Rob Herring <robh@kernel.org> Cc: devicetree@vger.kernel.org Cc: linux-wireless@vger.kernel.org Cc: netdev@vger.kernel.org --- .../net/wireless/microchip/wilc1000/sdio.c | 27 ++++++++++++++----- 1 file changed, 21 insertions(+), 6 deletions(-)