| Message ID | 55E6F497.8080300@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 2015-09-02 at 18:37 +0530, Vaibhav Hiremath wrote: > On Wednesday 02 September 2015 02:07 AM, Joe Perches wrote: > > On Wed, 2015-09-02 at 00:54 +0530, Vaibhav Hiremath wrote: > >> Return value would give clear information about the actual root-cause > >> of the failure. > >> diff --git a/drivers/mmc/host/sdhci-pxav3.c b/drivers/mmc/host/sdhci-pxav3.c > >> @@ -455,7 +455,7 @@ static int sdhci_pxav3_probe(struct platform_device *pdev) > >> > >> ret = sdhci_add_host(host); > >> if (ret) { > >> - dev_err(&pdev->dev, "failed to add host\n"); > >> + dev_err(&pdev->dev, "failed to add host ret - %d\n", ret); > >> goto err_add_host; > >> } > > > > If this is really desirable, there are many other callers of > > sdhci_add_host with error messages just like this one. > > > How about this? If you are ok, I can change it and submit the patch > again. > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c [] > @@ -3176,8 +3176,11 @@ int sdhci_add_host(struct sdhci_host *host) > mmc->caps |= MMC_CAP_NEEDS_POLL; > > /* If there are external regulators, get them */ > - if (mmc_regulator_get_supply(mmc) == -EPROBE_DEFER) > + if (mmc_regulator_get_supply(mmc) == -EPROBE_DEFER) { > + pr_err("%s: regulator supply unavailable, deferring > probe. \n", > + mmc_hostname(mmc)); > return -EPROBE_DEFER; > + } (your email client has inappropriate line wrapping) The KERN_<LEVEL> here probably isn't right. Deferring isn't an error, at best it's a notification and perhaps should be at pr_notice/KERN_NOTICE I don't know how often or how many times this deferral can occur. Do you? If it's moderately common, that message should likely be ratelimited if it exists at all.
On Wednesday 02 September 2015 08:37 PM, Joe Perches wrote: > On Wed, 2015-09-02 at 18:37 +0530, Vaibhav Hiremath wrote: >> On Wednesday 02 September 2015 02:07 AM, Joe Perches wrote: >>> On Wed, 2015-09-02 at 00:54 +0530, Vaibhav Hiremath wrote: >>>> Return value would give clear information about the actual root-cause >>>> of the failure. >>>> diff --git a/drivers/mmc/host/sdhci-pxav3.c b/drivers/mmc/host/sdhci-pxav3.c >>>> @@ -455,7 +455,7 @@ static int sdhci_pxav3_probe(struct platform_device *pdev) >>>> >>>> ret = sdhci_add_host(host); >>>> if (ret) { >>>> - dev_err(&pdev->dev, "failed to add host\n"); >>>> + dev_err(&pdev->dev, "failed to add host ret - %d\n", ret); >>>> goto err_add_host; >>>> } >>> >>> If this is really desirable, there are many other callers of >>> sdhci_add_host with error messages just like this one. >>> >> How about this? If you are ok, I can change it and submit the patch >> again. >> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c > [] >> @@ -3176,8 +3176,11 @@ int sdhci_add_host(struct sdhci_host *host) >> mmc->caps |= MMC_CAP_NEEDS_POLL; >> >> /* If there are external regulators, get them */ >> - if (mmc_regulator_get_supply(mmc) == -EPROBE_DEFER) >> + if (mmc_regulator_get_supply(mmc) == -EPROBE_DEFER) { >> + pr_err("%s: regulator supply unavailable, deferring >> probe. \n", >> + mmc_hostname(mmc)); >> return -EPROBE_DEFER; >> + } > > (your email client has inappropriate line wrapping) > > The KERN_<LEVEL> here probably isn't right. > > Deferring isn't an error, at best it's a notification I would consider it as an ERROR if it gets deferred continuously/multiple times due to same reason. > and perhaps should be at pr_notice/KERN_NOTICE > Yeah, KERN_NOTICE looks right here. > I don't know how often or how many times this deferral > can occur. Do you? > -EDEFER_PROBE usually means that driver has some dependency, for which it has to wait. In my case, during every boot, I pxav3_sdhci_probe gets deferred once due to regulator unavailability. Thanks, Vaibhav
On 2 September 2015 at 17:16, Vaibhav Hiremath <vaibhav.hiremath@linaro.org> wrote: > > > On Wednesday 02 September 2015 08:37 PM, Joe Perches wrote: >> >> On Wed, 2015-09-02 at 18:37 +0530, Vaibhav Hiremath wrote: >>> >>> On Wednesday 02 September 2015 02:07 AM, Joe Perches wrote: >>>> >>>> On Wed, 2015-09-02 at 00:54 +0530, Vaibhav Hiremath wrote: >>>>> >>>>> Return value would give clear information about the actual root-cause >>>>> of the failure. >>>>> diff --git a/drivers/mmc/host/sdhci-pxav3.c >>>>> b/drivers/mmc/host/sdhci-pxav3.c >>>>> @@ -455,7 +455,7 @@ static int sdhci_pxav3_probe(struct platform_device >>>>> *pdev) >>>>> >>>>> ret = sdhci_add_host(host); >>>>> if (ret) { >>>>> - dev_err(&pdev->dev, "failed to add host\n"); >>>>> + dev_err(&pdev->dev, "failed to add host ret - %d\n", >>>>> ret); >>>>> goto err_add_host; >>>>> } >>>> >>>> >>>> If this is really desirable, there are many other callers of >>>> sdhci_add_host with error messages just like this one. >>>> >>> How about this? If you are ok, I can change it and submit the patch >>> again. >>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c >> >> [] >>> >>> @@ -3176,8 +3176,11 @@ int sdhci_add_host(struct sdhci_host *host) >>> mmc->caps |= MMC_CAP_NEEDS_POLL; >>> >>> /* If there are external regulators, get them */ >>> - if (mmc_regulator_get_supply(mmc) == -EPROBE_DEFER) >>> + if (mmc_regulator_get_supply(mmc) == -EPROBE_DEFER) { >>> + pr_err("%s: regulator supply unavailable, deferring >>> probe. \n", >>> + mmc_hostname(mmc)); >>> return -EPROBE_DEFER; >>> + } >> >> >> (your email client has inappropriate line wrapping) >> >> The KERN_<LEVEL> here probably isn't right. >> >> Deferring isn't an error, at best it's a notification > > > I would consider it as an ERROR if it gets deferred > continuously/multiple times due to same reason. > >> and perhaps should be at pr_notice/KERN_NOTICE >> > > Yeah, KERN_NOTICE looks right here. > >> I don't know how often or how many times this deferral >> can occur. Do you? >> > > -EDEFER_PROBE usually means that driver has some dependency, > for which it has to wait. > In my case, during every boot, I pxav3_sdhci_probe gets deferred once > due to regulator unavailability. The driver core already prints a message on the debug level for -EDEFER_PROBE. Please don't do that in the driver as well. Kind regards Uffe
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c index d2caa60..3a4902c 100644 --- a/drivers/mmc/host/sdhci.c +++ b/drivers/mmc/host/sdhci.c @@ -3176,8 +3176,11 @@ int sdhci_add_host(struct sdhci_host *host) mmc->caps |= MMC_CAP_NEEDS_POLL; /* If there are external regulators, get them */ - if (mmc_regulator_get_supply(mmc) == -EPROBE_DEFER) + if (mmc_regulator_get_supply(mmc) == -EPROBE_DEFER) { + pr_err("%s: regulator supply unavailable, deferring probe. \n", + mmc_hostname(mmc)); return -EPROBE_DEFER; + } /* If vqmmc regulator and no 1.8V signalling, then there's no