| Message ID | 20221220015254.796568-1-marex@denx.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] mmc: sdhci: Always apply sdhci-caps-mask and sdhci-caps to caps | expand |
> -----Original Message----- > From: Marek Vasut <marex@denx.de> > Sent: 2022年12月20日 9:53 > To: linux-mmc@vger.kernel.org > Cc: Marek Vasut <marex@denx.de>; Adrian Hunter <adrian.hunter@intel.com>; > Ulf Hansson <ulf.hansson@linaro.org>; Zach Brown <zach.brown@ni.com> > Subject: [PATCH] [RFC] mmc: sdhci: Always apply sdhci-caps-mask and > sdhci-caps to caps > > The original implementation in the commit referenced below only modifies > caps in case no caps are passed to sdhci_read_caps() via parameter, this does > not seem correct. Always modify the caps according to the properties from DT. > > 92e0c44b92e4 ("mmc: sdhci: Use sdhci-caps-mask and sdhci-caps to change Need to add Fixes as below: Fixes: 92e0c44b92e4 ("mmc: sdhci: Use sdhci-caps-mask and sdhci-caps to change the caps read during __sdhci_read_caps") I did a grep under the /drivers/mmc/host, seems all callers use NULL for the parameter caps and caps1, So maybe we could just simplify the code like this: diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c index f3af1bd0f7b9..0ed8c5b36ecb 100644 --- a/drivers/mmc/host/sdhci.c +++ b/drivers/mmc/host/sdhci.c @@ -4090,8 +4090,7 @@ static int sdhci_set_dma_mask(struct sdhci_host *host) return ret; } -void __sdhci_read_caps(struct sdhci_host *host, const u16 *ver, - const u32 *caps, const u32 *caps1) +void __sdhci_read_caps(struct sdhci_host *host, const u16 *ver) { u16 v; u64 dt_caps_mask = 0; @@ -4124,24 +4123,16 @@ void __sdhci_read_caps(struct sdhci_host *host, const u16 *ver, if (host->quirks & SDHCI_QUIRK_MISSING_CAPS) return; - if (caps) { - host->caps = *caps; - } else { - host->caps = sdhci_readl(host, SDHCI_CAPABILITIES); - host->caps &= ~lower_32_bits(dt_caps_mask); - host->caps |= lower_32_bits(dt_caps); - } + host->caps = sdhci_readl(host, SDHCI_CAPABILITIES); + host->caps &= ~lower_32_bits(dt_caps_mask); + host->caps |= lower_32_bits(dt_caps); if (host->version < SDHCI_SPEC_300) return; - if (caps1) { - host->caps1 = *caps1; - } else { - host->caps1 = sdhci_readl(host, SDHCI_CAPABILITIES_1); - host->caps1 &= ~upper_32_bits(dt_caps_mask); - host->caps1 |= upper_32_bits(dt_caps); - } + host->caps1 = sdhci_readl(host, SDHCI_CAPABILITIES_1); + host->caps1 &= ~upper_32_bits(dt_caps_mask); + host->caps1 |= upper_32_bits(dt_caps); } EXPORT_SYMBOL_GPL(__sdhci_read_caps); Best Regards Haibo Chen > the caps read during __sdhci_read_caps") > Signed-off-by: Marek Vasut <marex@denx.de> > --- > Cc: Adrian Hunter <adrian.hunter@intel.com> > Cc: Ulf Hansson <ulf.hansson@linaro.org> > Cc: Zach Brown <zach.brown@ni.com> > To: linux-mmc@vger.kernel.org > --- > Note: I am sending it as an RFC, because it seems I might be missing > something obvious. > --- > drivers/mmc/host/sdhci.c | 20 ++++++-------------- > 1 file changed, 6 insertions(+), 14 deletions(-) > > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c index > f3af1bd0f7b95..52719d3118ffd 100644 > --- a/drivers/mmc/host/sdhci.c > +++ b/drivers/mmc/host/sdhci.c > @@ -4124,24 +4124,16 @@ void __sdhci_read_caps(struct sdhci_host *host, > const u16 *ver, > if (host->quirks & SDHCI_QUIRK_MISSING_CAPS) > return; > > - if (caps) { > - host->caps = *caps; > - } else { > - host->caps = sdhci_readl(host, SDHCI_CAPABILITIES); > - host->caps &= ~lower_32_bits(dt_caps_mask); > - host->caps |= lower_32_bits(dt_caps); > - } > + host->caps = caps ? *caps : sdhci_readl(host, SDHCI_CAPABILITIES); > + host->caps &= ~lower_32_bits(dt_caps_mask); > + host->caps |= lower_32_bits(dt_caps); > > if (host->version < SDHCI_SPEC_300) > return; > > - if (caps1) { > - host->caps1 = *caps1; > - } else { > - host->caps1 = sdhci_readl(host, SDHCI_CAPABILITIES_1); > - host->caps1 &= ~upper_32_bits(dt_caps_mask); > - host->caps1 |= upper_32_bits(dt_caps); > - } > + host->caps1 = caps1 ? *caps1 : sdhci_readl(host, SDHCI_CAPABILITIES_1); > + host->caps1 &= ~upper_32_bits(dt_caps_mask); > + host->caps1 |= upper_32_bits(dt_caps); > } > EXPORT_SYMBOL_GPL(__sdhci_read_caps); > > -- > 2.35.1
On 12/20/22 04:04, Bough Chen wrote: >> -----Original Message----- >> From: Marek Vasut <marex@denx.de> >> Sent: 2022年12月20日 9:53 >> To: linux-mmc@vger.kernel.org >> Cc: Marek Vasut <marex@denx.de>; Adrian Hunter <adrian.hunter@intel.com>; >> Ulf Hansson <ulf.hansson@linaro.org>; Zach Brown <zach.brown@ni.com> >> Subject: [PATCH] [RFC] mmc: sdhci: Always apply sdhci-caps-mask and >> sdhci-caps to caps >> >> The original implementation in the commit referenced below only modifies >> caps in case no caps are passed to sdhci_read_caps() via parameter, this does >> not seem correct. Always modify the caps according to the properties from DT. >> >> 92e0c44b92e4 ("mmc: sdhci: Use sdhci-caps-mask and sdhci-caps to change > > Need to add Fixes as below: > Fixes: 92e0c44b92e4 ("mmc: sdhci: Use sdhci-caps-mask and sdhci-caps to change the caps read during __sdhci_read_caps") > > I did a grep under the /drivers/mmc/host, seems all callers use NULL for the parameter caps and caps1, > So maybe we could just simplify the code like this: That would make backporting harder, so subsequent patch please. But I am more interested in knowing whether this change even makes sense, since it was broken for like 6 years and nobody noticed.
On 20/12/22 05:42, Marek Vasut wrote: > On 12/20/22 04:04, Bough Chen wrote: >>> -----Original Message----- >>> From: Marek Vasut <marex@denx.de> >>> Sent: 2022年12月20日 9:53 >>> To: linux-mmc@vger.kernel.org >>> Cc: Marek Vasut <marex@denx.de>; Adrian Hunter <adrian.hunter@intel.com>; >>> Ulf Hansson <ulf.hansson@linaro.org>; Zach Brown <zach.brown@ni.com> >>> Subject: [PATCH] [RFC] mmc: sdhci: Always apply sdhci-caps-mask and >>> sdhci-caps to caps >>> >>> The original implementation in the commit referenced below only modifies >>> caps in case no caps are passed to sdhci_read_caps() via parameter, this does >>> not seem correct. Always modify the caps according to the properties from DT. >>> >>> 92e0c44b92e4 ("mmc: sdhci: Use sdhci-caps-mask and sdhci-caps to change >> >> Need to add Fixes as below: >> Fixes: 92e0c44b92e4 ("mmc: sdhci: Use sdhci-caps-mask and sdhci-caps to change the caps read during __sdhci_read_caps") >> >> I did a grep under the /drivers/mmc/host, seems all callers use NULL for the parameter caps and caps1, >> So maybe we could just simplify the code like this: > > That would make backporting harder, so subsequent patch please. > > But I am more interested in knowing whether this change even makes sense, since it was broken for like 6 years and nobody noticed. If all drivers pass NULL then the patch does not change anything and consequently is not a fix, and a fixes tag is not appropriate. The caps and caps1 variables were added to allow __sdhci_read_caps() to be used to remove the use of SDHCI_QUIRK_MISSING_CAPS, so I would leave them for now and I will try to finally send some patches to actually do that. Otherwise the change seems reasonable as is, except for the commit message which could just be something like: The original implementation in commit 92e0c44b92e4 ("mmc: sdhci: Use sdhci-caps-mask and sdhci-caps to change the caps read during __sdhci_read_caps") only modifies caps in case no caps are passed to sdhci_read_caps() via parameter, this does not seem correct. Always modify the caps according to the properties from DT.
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c index f3af1bd0f7b95..52719d3118ffd 100644 --- a/drivers/mmc/host/sdhci.c +++ b/drivers/mmc/host/sdhci.c @@ -4124,24 +4124,16 @@ void __sdhci_read_caps(struct sdhci_host *host, const u16 *ver, if (host->quirks & SDHCI_QUIRK_MISSING_CAPS) return; - if (caps) { - host->caps = *caps; - } else { - host->caps = sdhci_readl(host, SDHCI_CAPABILITIES); - host->caps &= ~lower_32_bits(dt_caps_mask); - host->caps |= lower_32_bits(dt_caps); - } + host->caps = caps ? *caps : sdhci_readl(host, SDHCI_CAPABILITIES); + host->caps &= ~lower_32_bits(dt_caps_mask); + host->caps |= lower_32_bits(dt_caps); if (host->version < SDHCI_SPEC_300) return; - if (caps1) { - host->caps1 = *caps1; - } else { - host->caps1 = sdhci_readl(host, SDHCI_CAPABILITIES_1); - host->caps1 &= ~upper_32_bits(dt_caps_mask); - host->caps1 |= upper_32_bits(dt_caps); - } + host->caps1 = caps1 ? *caps1 : sdhci_readl(host, SDHCI_CAPABILITIES_1); + host->caps1 &= ~upper_32_bits(dt_caps_mask); + host->caps1 |= upper_32_bits(dt_caps); } EXPORT_SYMBOL_GPL(__sdhci_read_caps);
The original implementation in the commit referenced below only modifies caps in case no caps are passed to sdhci_read_caps() via parameter, this does not seem correct. Always modify the caps according to the properties from DT. 92e0c44b92e4 ("mmc: sdhci: Use sdhci-caps-mask and sdhci-caps to change the caps read during __sdhci_read_caps") Signed-off-by: Marek Vasut <marex@denx.de> --- Cc: Adrian Hunter <adrian.hunter@intel.com> Cc: Ulf Hansson <ulf.hansson@linaro.org> Cc: Zach Brown <zach.brown@ni.com> To: linux-mmc@vger.kernel.org --- Note: I am sending it as an RFC, because it seems I might be missing something obvious. --- drivers/mmc/host/sdhci.c | 20 ++++++-------------- 1 file changed, 6 insertions(+), 14 deletions(-)