[bug report] mmc: core: Re-work HW reset for SDIO cards

[bug report] mmc: core: Re-work HW reset for SDIO cards

[Dan Carpenter]

Hello Ulf Hansson,

The patch 2ac55d5e5ec9: "mmc: core: Re-work HW reset for SDIO cards"
from Oct 17, 2019, leads to the following static checker warning:

	drivers/net/wireless/ath/ath10k/sdio.c:1521 ath10k_sdio_hif_power_down()
	warn: 'ret' can be either negative or positive

drivers/net/wireless/ath/ath10k/sdio.c
  1495  static void ath10k_sdio_hif_power_down(struct ath10k *ar)
  1496  {
  1497          struct ath10k_sdio *ar_sdio = ath10k_sdio_priv(ar);
  1498          int ret;
  1499  
  1500          if (ar_sdio->is_disabled)
  1501                  return;
  1502  
  1503          ath10k_dbg(ar, ATH10K_DBG_BOOT, "sdio power off\n");
  1504  
  1505          /* Disable the card */
  1506          sdio_claim_host(ar_sdio->func);
  1507  
  1508          ret = sdio_disable_func(ar_sdio->func);
  1509          if (ret) {
  1510                  ath10k_warn(ar, "unable to disable sdio function: %d\n", ret);
  1511                  sdio_release_host(ar_sdio->func);
  1512                  return;
  1513          }
  1514  
  1515          ret = mmc_hw_reset(ar_sdio->func->card->host);
  1516          if (ret)

It used to be that mmc_hw_reset() return negative error codes or zero
but now it returns 1 on certain success paths.

  1517                  ath10k_warn(ar, "unable to reset sdio: %d\n", ret);
  1518  
  1519          sdio_release_host(ar_sdio->func);
  1520  
  1521          ar_sdio->is_disabled = true;
  1522  }


regards,
dan carpenter

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [bug report] mmc: core: Re-work HW reset for SDIO cards

[Ulf Hansson]

+ Kalle

On Fri, 13 Dec 2019 at 19:51, Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> Hello Ulf Hansson,
>
> The patch 2ac55d5e5ec9: "mmc: core: Re-work HW reset for SDIO cards"
> from Oct 17, 2019, leads to the following static checker warning:
>
>         drivers/net/wireless/ath/ath10k/sdio.c:1521 ath10k_sdio_hif_power_down()
>         warn: 'ret' can be either negative or positive

Thanks for reporting!

>
> drivers/net/wireless/ath/ath10k/sdio.c
>   1495  static void ath10k_sdio_hif_power_down(struct ath10k *ar)
>   1496  {
>   1497          struct ath10k_sdio *ar_sdio = ath10k_sdio_priv(ar);
>   1498          int ret;
>   1499
>   1500          if (ar_sdio->is_disabled)
>   1501                  return;
>   1502
>   1503          ath10k_dbg(ar, ATH10K_DBG_BOOT, "sdio power off\n");
>   1504
>   1505          /* Disable the card */
>   1506          sdio_claim_host(ar_sdio->func);
>   1507
>   1508          ret = sdio_disable_func(ar_sdio->func);
>   1509          if (ret) {
>   1510                  ath10k_warn(ar, "unable to disable sdio function: %d\n", ret);
>   1511                  sdio_release_host(ar_sdio->func);
>   1512                  return;
>   1513          }
>   1514
>   1515          ret = mmc_hw_reset(ar_sdio->func->card->host);
>   1516          if (ret)
>
> It used to be that mmc_hw_reset() return negative error codes or zero
> but now it returns 1 on certain success paths.

Correct.

I was actually looking into this while changing the behaviour of
mmc_hw_reset(). However I decided to leave this as is.

The main reason is, that mmc_hw_reset() is not going to power down the
card. It's hard power cycle, so I am kind of surprised that is being
used at all in this path. This in combination of expecting the value
from mmc_hw_reset() to never be 1 here, seemed like a good idea to
preserve the logging of the warning message.

To silent the static checker tool from warning, we could check
explicitly for "1". Is that something you want me to do?

>
>   1517                  ath10k_warn(ar, "unable to reset sdio: %d\n", ret);
>   1518
>   1519          sdio_release_host(ar_sdio->func);
>   1520
>   1521          ar_sdio->is_disabled = true;
>   1522  }
>
>
> regards,
> dan carpenter

Kind regards
Uffe

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [bug report] mmc: core: Re-work HW reset for SDIO cards

[Dan Carpenter]

On Tue, Dec 17, 2019 at 08:54:47AM +0100, Ulf Hansson wrote:
> + Kalle
> 
> On Fri, 13 Dec 2019 at 19:51, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> >
> > Hello Ulf Hansson,
> >
> > The patch 2ac55d5e5ec9: "mmc: core: Re-work HW reset for SDIO cards"
> > from Oct 17, 2019, leads to the following static checker warning:
> >
> >         drivers/net/wireless/ath/ath10k/sdio.c:1521 ath10k_sdio_hif_power_down()
> >         warn: 'ret' can be either negative or positive
> 
> Thanks for reporting!
> 
> >
> > drivers/net/wireless/ath/ath10k/sdio.c
> >   1495  static void ath10k_sdio_hif_power_down(struct ath10k *ar)
> >   1496  {
> >   1497          struct ath10k_sdio *ar_sdio = ath10k_sdio_priv(ar);
> >   1498          int ret;
> >   1499
> >   1500          if (ar_sdio->is_disabled)
> >   1501                  return;
> >   1502
> >   1503          ath10k_dbg(ar, ATH10K_DBG_BOOT, "sdio power off\n");
> >   1504
> >   1505          /* Disable the card */
> >   1506          sdio_claim_host(ar_sdio->func);
> >   1507
> >   1508          ret = sdio_disable_func(ar_sdio->func);
> >   1509          if (ret) {
> >   1510                  ath10k_warn(ar, "unable to disable sdio function: %d\n", ret);
> >   1511                  sdio_release_host(ar_sdio->func);
> >   1512                  return;
> >   1513          }
> >   1514
> >   1515          ret = mmc_hw_reset(ar_sdio->func->card->host);
> >   1516          if (ret)
> >
> > It used to be that mmc_hw_reset() return negative error codes or zero
> > but now it returns 1 on certain success paths.
> 
> Correct.
> 
> I was actually looking into this while changing the behaviour of
> mmc_hw_reset(). However I decided to leave this as is.
> 
> The main reason is, that mmc_hw_reset() is not going to power down the
> card. It's hard power cycle, so I am kind of surprised that is being
> used at all in this path. This in combination of expecting the value
> from mmc_hw_reset() to never be 1 here, seemed like a good idea to
> preserve the logging of the warning message.
> 
> To silent the static checker tool from warning, we could check
> explicitly for "1". Is that something you want me to do?

I feel like checking for 1 would be more readable for humans as well.

Or we could just leave it as-is.  I'm not likely to ever publish this
static checker warning.

regards,
dan carpenter


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k