| Message ID | 245d569e4c258063dbd78bd30c7027638b30f059.1577960737.git.mirq-linux@rere.qmqm.pl |
|---|---|
| State | Superseded |
| Headers | show |
| Series | mmc: tegra: fix SDR50 tuning override | expand |
| Context | Check | Description |
|---|---|---|
| tagr/GVS | pending | None |
| tagr/GVS | success | None |
On Thu, Jan 02, 2020 at 11:30:50AM +0100, Michał Mirosław wrote: > Commit 7ad2ed1dfcbe inadvertently mixed up a quirk flag's name and > broke SDR50 tuning override. Use correct NVQUIRK_ name. > > Fixes: 7ad2ed1dfcbe ("mmc: tegra: enable UHS-I modes") > Depends-on: 4f6aa3264af4 ("mmc: tegra: Only advertise UHS modes if IO regulator is present") > Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl> > --- > drivers/mmc/host/sdhci-tegra.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Oh my... good catch! Reviewed-by: Thierry Reding <treding@nvidia.com> I also ran this through our internal test system and all tests pass, so also: Tested-by: Thierry Reding <treding@nvidia.com> I'm not sure if that "Depends-on:" tag is anything that's recognized anywhere. It might be better to turn that into an additional "Fixes:" line. Adding Greg to see if he has a standard way of dealing with this kind of dependency. Greg, what's your preferred way to handle these situations? I think the intention here was to describe that the original error was introduced by commit 7ad2ed1dfcbe ("mmc: tegra: enable UHS-I modes"), but then commit 4f6aa3264af4 ("mmc: tegra: Only advertise UHS modes if IO regulator is present") moved that code around, so this patch here will only be back- portable until the latter commit, but should be backported until the former. I'm not sure it matters in this case because the latter went into v4.5 and the former went into v4.8, so we don't need this in v4.4 and v4.9 should already have all the prerequisites for this. But in general, do you need anything special to model such dependencies, or is this simply solved by requiring a manual backport if the fix stops applying cleanly at some point? Thierry
On Mon, Jan 06, 2020 at 01:07:18PM +0100, Thierry Reding wrote: > On Thu, Jan 02, 2020 at 11:30:50AM +0100, Michał Mirosław wrote: > > Commit 7ad2ed1dfcbe inadvertently mixed up a quirk flag's name and > > broke SDR50 tuning override. Use correct NVQUIRK_ name. > > > > Fixes: 7ad2ed1dfcbe ("mmc: tegra: enable UHS-I modes") > > Depends-on: 4f6aa3264af4 ("mmc: tegra: Only advertise UHS modes if IO regulator is present") > > Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl> > > --- > > drivers/mmc/host/sdhci-tegra.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > Oh my... good catch! > > Reviewed-by: Thierry Reding <treding@nvidia.com> > > I also ran this through our internal test system and all tests pass, so > also: > > Tested-by: Thierry Reding <treding@nvidia.com> > > I'm not sure if that "Depends-on:" tag is anything that's recognized > anywhere. It might be better to turn that into an additional "Fixes:" > line. Adding Greg to see if he has a standard way of dealing with this > kind of dependency. > > Greg, what's your preferred way to handle these situations? I think the > intention here was to describe that the original error was introduced by > commit 7ad2ed1dfcbe ("mmc: tegra: enable UHS-I modes"), but then commit > 4f6aa3264af4 ("mmc: tegra: Only advertise UHS modes if IO regulator is > present") moved that code around, so this patch here will only be back- > portable until the latter commit, but should be backported until the > former. The stable kernel rules document says how to handle this, but the "depends on" commit id in the comment to the right of the stable@k.o cc: line in the changelog area. thanks, greg k-h
On Mon, Jan 06, 2020 at 01:27:45PM +0100, Greg Kroah-Hartman wrote: > On Mon, Jan 06, 2020 at 01:07:18PM +0100, Thierry Reding wrote: > > On Thu, Jan 02, 2020 at 11:30:50AM +0100, Michał Mirosław wrote: > > > Commit 7ad2ed1dfcbe inadvertently mixed up a quirk flag's name and > > > broke SDR50 tuning override. Use correct NVQUIRK_ name. > > > > > > Fixes: 7ad2ed1dfcbe ("mmc: tegra: enable UHS-I modes") > > > Depends-on: 4f6aa3264af4 ("mmc: tegra: Only advertise UHS modes if IO regulator is present") > > > Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl> > > > --- > > > drivers/mmc/host/sdhci-tegra.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > Oh my... good catch! > > > > Reviewed-by: Thierry Reding <treding@nvidia.com> > > > > I also ran this through our internal test system and all tests pass, so > > also: > > > > Tested-by: Thierry Reding <treding@nvidia.com> > > > > I'm not sure if that "Depends-on:" tag is anything that's recognized > > anywhere. It might be better to turn that into an additional "Fixes:" > > line. Adding Greg to see if he has a standard way of dealing with this > > kind of dependency. > > > > Greg, what's your preferred way to handle these situations? I think the > > intention here was to describe that the original error was introduced by > > commit 7ad2ed1dfcbe ("mmc: tegra: enable UHS-I modes"), but then commit > > 4f6aa3264af4 ("mmc: tegra: Only advertise UHS modes if IO regulator is > > present") moved that code around, so this patch here will only be back- > > portable until the latter commit, but should be backported until the > > former. > > The stable kernel rules document says how to handle this, but the > "depends on" commit id in the comment to the right of the stable@k.o cc: > line in the changelog area. That only mentions "static" prerequisites needed by the patch, but what if the prerequisites change depending on version? Could I do something like this: Cc: <stable@vger.kernel.org> # 4.4.x: abcdef: ... Cc: <stable@vger.kernel.org> # 4.9.x: bcdefa: ... Cc: <stable@vger.kernel.org> Would that mean that the patch is selected for all stable releases (because of the last line with no version prerequisite) but when applied for stable-4.4 the abcdef patch gets pulled in and for stable-4.9 the bcdefa dependency is applied before the patch? I suppose this is perhaps a bit of an exotic case, but it might be good to document it specifically because it might be fairly rare. I can draft a change if you think this is useful to add. Thierry
On Mon, Jan 06, 2020 at 02:37:03PM +0100, Thierry Reding wrote: > On Mon, Jan 06, 2020 at 01:27:45PM +0100, Greg Kroah-Hartman wrote: > > On Mon, Jan 06, 2020 at 01:07:18PM +0100, Thierry Reding wrote: > > > On Thu, Jan 02, 2020 at 11:30:50AM +0100, Michał Mirosław wrote: > > > > Commit 7ad2ed1dfcbe inadvertently mixed up a quirk flag's name and > > > > broke SDR50 tuning override. Use correct NVQUIRK_ name. > > > > > > > > Fixes: 7ad2ed1dfcbe ("mmc: tegra: enable UHS-I modes") > > > > Depends-on: 4f6aa3264af4 ("mmc: tegra: Only advertise UHS modes if IO regulator is present") > > > > Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl> > > > > --- > > > > drivers/mmc/host/sdhci-tegra.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > Oh my... good catch! > > > > > > Reviewed-by: Thierry Reding <treding@nvidia.com> > > > > > > I also ran this through our internal test system and all tests pass, so > > > also: > > > > > > Tested-by: Thierry Reding <treding@nvidia.com> > > > > > > I'm not sure if that "Depends-on:" tag is anything that's recognized > > > anywhere. It might be better to turn that into an additional "Fixes:" > > > line. Adding Greg to see if he has a standard way of dealing with this > > > kind of dependency. > > > > > > Greg, what's your preferred way to handle these situations? I think the > > > intention here was to describe that the original error was introduced by > > > commit 7ad2ed1dfcbe ("mmc: tegra: enable UHS-I modes"), but then commit > > > 4f6aa3264af4 ("mmc: tegra: Only advertise UHS modes if IO regulator is > > > present") moved that code around, so this patch here will only be back- > > > portable until the latter commit, but should be backported until the > > > former. > > > > The stable kernel rules document says how to handle this, but the > > "depends on" commit id in the comment to the right of the stable@k.o cc: > > line in the changelog area. > > That only mentions "static" prerequisites needed by the patch, but what > if the prerequisites change depending on version? > > Could I do something like this: > > Cc: <stable@vger.kernel.org> # 4.4.x: abcdef: ... > Cc: <stable@vger.kernel.org> # 4.9.x: bcdefa: ... > Cc: <stable@vger.kernel.org> Yes. > Would that mean that the patch is selected for all stable releases > (because of the last line with no version prerequisite) but when applied > for stable-4.4 the abcdef patch gets pulled in and for stable-4.9 the > bcdefa dependency is applied before the patch? Yes. > I suppose this is perhaps a bit of an exotic case, but it might be good > to document it specifically because it might be fairly rare. I can draft > a change if you think this is useful to add. I thought this was already in there, as others have done it in the past. It's a _very_ exotic case, I wouldn't worry about it, just document it like this, and if I have problems applying the patches to stable I'll be sure to let you know and you can always tell me then. That's usually the easiest thing to do anyway :) thanks, greg k-h
On Tue, Jan 07, 2020 at 10:37:15AM +0100, Greg Kroah-Hartman wrote: > On Mon, Jan 06, 2020 at 02:37:03PM +0100, Thierry Reding wrote: > > On Mon, Jan 06, 2020 at 01:27:45PM +0100, Greg Kroah-Hartman wrote: > > > On Mon, Jan 06, 2020 at 01:07:18PM +0100, Thierry Reding wrote: > > > > On Thu, Jan 02, 2020 at 11:30:50AM +0100, Michał Mirosław wrote: > > > > > Commit 7ad2ed1dfcbe inadvertently mixed up a quirk flag's name and > > > > > broke SDR50 tuning override. Use correct NVQUIRK_ name. > > > > > > > > > > Fixes: 7ad2ed1dfcbe ("mmc: tegra: enable UHS-I modes") > > > > > Depends-on: 4f6aa3264af4 ("mmc: tegra: Only advertise UHS modes if IO regulator is present") > > > > > Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl> > > > > > --- > > > > > drivers/mmc/host/sdhci-tegra.c | 2 +- > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > Oh my... good catch! > > > > > > > > Reviewed-by: Thierry Reding <treding@nvidia.com> > > > > > > > > I also ran this through our internal test system and all tests pass, so > > > > also: > > > > > > > > Tested-by: Thierry Reding <treding@nvidia.com> > > > > > > > > I'm not sure if that "Depends-on:" tag is anything that's recognized > > > > anywhere. It might be better to turn that into an additional "Fixes:" > > > > line. Adding Greg to see if he has a standard way of dealing with this > > > > kind of dependency. > > > > > > > > Greg, what's your preferred way to handle these situations? I think the > > > > intention here was to describe that the original error was introduced by > > > > commit 7ad2ed1dfcbe ("mmc: tegra: enable UHS-I modes"), but then commit > > > > 4f6aa3264af4 ("mmc: tegra: Only advertise UHS modes if IO regulator is > > > > present") moved that code around, so this patch here will only be back- > > > > portable until the latter commit, but should be backported until the > > > > former. > > > > > > The stable kernel rules document says how to handle this, but the > > > "depends on" commit id in the comment to the right of the stable@k.o cc: > > > line in the changelog area. > > > > That only mentions "static" prerequisites needed by the patch, but what > > if the prerequisites change depending on version? > > > > Could I do something like this: > > > > Cc: <stable@vger.kernel.org> # 4.4.x: abcdef: ... > > Cc: <stable@vger.kernel.org> # 4.9.x: bcdefa: ... > > Cc: <stable@vger.kernel.org> > > Yes. > > > Would that mean that the patch is selected for all stable releases > > (because of the last line with no version prerequisite) but when applied > > for stable-4.4 the abcdef patch gets pulled in and for stable-4.9 the > > bcdefa dependency is applied before the patch? > > Yes. > > > I suppose this is perhaps a bit of an exotic case, but it might be good > > to document it specifically because it might be fairly rare. I can draft > > a change if you think this is useful to add. > > I thought this was already in there, as others have done it in the past. > > It's a _very_ exotic case, I wouldn't worry about it, just document it > like this, and if I have problems applying the patches to stable I'll be > sure to let you know and you can always tell me then. That's usually > the easiest thing to do anyway :) I understood the wording in stable-kernel-rules.rst as meaning that comments on Cc: lines make mentioned commit pulled in (cherry-picked). In this case I think this is ok, but in case the pulled-in patch changes something else (the dependency is only because of touching nearby code), how would I specify this and avoid the hint to include the other patch? Best Regards, Michał Mirosław
On Tue, Jan 07, 2020 at 10:53:59AM +0100, Michał Mirosław wrote: > On Tue, Jan 07, 2020 at 10:37:15AM +0100, Greg Kroah-Hartman wrote: > > On Mon, Jan 06, 2020 at 02:37:03PM +0100, Thierry Reding wrote: > > > On Mon, Jan 06, 2020 at 01:27:45PM +0100, Greg Kroah-Hartman wrote: > > > > On Mon, Jan 06, 2020 at 01:07:18PM +0100, Thierry Reding wrote: > > > > > On Thu, Jan 02, 2020 at 11:30:50AM +0100, Michał Mirosław wrote: > > > > > > Commit 7ad2ed1dfcbe inadvertently mixed up a quirk flag's name and > > > > > > broke SDR50 tuning override. Use correct NVQUIRK_ name. > > > > > > > > > > > > Fixes: 7ad2ed1dfcbe ("mmc: tegra: enable UHS-I modes") > > > > > > Depends-on: 4f6aa3264af4 ("mmc: tegra: Only advertise UHS modes if IO regulator is present") > > > > > > Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl> > > > > > > --- > > > > > > drivers/mmc/host/sdhci-tegra.c | 2 +- > > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > Oh my... good catch! > > > > > > > > > > Reviewed-by: Thierry Reding <treding@nvidia.com> > > > > > > > > > > I also ran this through our internal test system and all tests pass, so > > > > > also: > > > > > > > > > > Tested-by: Thierry Reding <treding@nvidia.com> > > > > > > > > > > I'm not sure if that "Depends-on:" tag is anything that's recognized > > > > > anywhere. It might be better to turn that into an additional "Fixes:" > > > > > line. Adding Greg to see if he has a standard way of dealing with this > > > > > kind of dependency. > > > > > > > > > > Greg, what's your preferred way to handle these situations? I think the > > > > > intention here was to describe that the original error was introduced by > > > > > commit 7ad2ed1dfcbe ("mmc: tegra: enable UHS-I modes"), but then commit > > > > > 4f6aa3264af4 ("mmc: tegra: Only advertise UHS modes if IO regulator is > > > > > present") moved that code around, so this patch here will only be back- > > > > > portable until the latter commit, but should be backported until the > > > > > former. > > > > > > > > The stable kernel rules document says how to handle this, but the > > > > "depends on" commit id in the comment to the right of the stable@k.o cc: > > > > line in the changelog area. > > > > > > That only mentions "static" prerequisites needed by the patch, but what > > > if the prerequisites change depending on version? > > > > > > Could I do something like this: > > > > > > Cc: <stable@vger.kernel.org> # 4.4.x: abcdef: ... > > > Cc: <stable@vger.kernel.org> # 4.9.x: bcdefa: ... > > > Cc: <stable@vger.kernel.org> > > > > Yes. > > > > > Would that mean that the patch is selected for all stable releases > > > (because of the last line with no version prerequisite) but when applied > > > for stable-4.4 the abcdef patch gets pulled in and for stable-4.9 the > > > bcdefa dependency is applied before the patch? > > > > Yes. > > > > > I suppose this is perhaps a bit of an exotic case, but it might be good > > > to document it specifically because it might be fairly rare. I can draft > > > a change if you think this is useful to add. > > > > I thought this was already in there, as others have done it in the past. > > > > It's a _very_ exotic case, I wouldn't worry about it, just document it > > like this, and if I have problems applying the patches to stable I'll be > > sure to let you know and you can always tell me then. That's usually > > the easiest thing to do anyway :) > > I understood the wording in stable-kernel-rules.rst as meaning that > comments on Cc: lines make mentioned commit pulled in (cherry-picked). > In this case I think this is ok, but in case the pulled-in patch changes > something else (the dependency is only because of touching nearby code), > how would I specify this and avoid the hint to include the other patch? I really do not understand what you are asking for here. Worst case, just say: cc: stable... # 4.4.x and if you know that fails to apply or build, then just wait for my email saying something failed and then respond with the needed commit ids or backported patches. It's not rocket science, and this isn't all that automated, I _can_ handle free-form text :) thanks, greg k-h
diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.c index 7bc950520fd9..403ac44a7378 100644 --- a/drivers/mmc/host/sdhci-tegra.c +++ b/drivers/mmc/host/sdhci-tegra.c @@ -386,7 +386,7 @@ static void tegra_sdhci_reset(struct sdhci_host *host, u8 mask) misc_ctrl |= SDHCI_MISC_CTRL_ENABLE_DDR50; if (soc_data->nvquirks & NVQUIRK_ENABLE_SDR104) misc_ctrl |= SDHCI_MISC_CTRL_ENABLE_SDR104; - if (soc_data->nvquirks & SDHCI_MISC_CTRL_ENABLE_SDR50) + if (soc_data->nvquirks & NVQUIRK_ENABLE_SDR50) clk_ctrl |= SDHCI_CLOCK_CTRL_SDR50_TUNING_OVERRIDE; }
Commit 7ad2ed1dfcbe inadvertently mixed up a quirk flag's name and broke SDR50 tuning override. Use correct NVQUIRK_ name. Fixes: 7ad2ed1dfcbe ("mmc: tegra: enable UHS-I modes") Depends-on: 4f6aa3264af4 ("mmc: tegra: Only advertise UHS modes if IO regulator is present") Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl> --- drivers/mmc/host/sdhci-tegra.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)