| Message ID | 20230130110721.111776-1-damien.lemoal@opensource.wdc.com |
|---|---|
| State | New |
| Headers | show |
| Series | ata: libata: Fix sata_down_spd_limit() when no link speed is reported | expand |
On Mon, Jan 30, 2023 at 08:07:21PM +0900, Damien Le Moal wrote: > Commit 2dc0b46b5ea3 ("libata: sata_down_spd_limit should return if > driver has not recorded sstatus speed") changed the behavior of > sata_down_spd_limit() to return doing nothing if a drive does not report > a current link speed, to avoid reducing the link speed to the lowest 1.5 > Gbps speed. > > However, the change assumed that a speed was recorded before probing > (e.g. before a suspend/resume) and set in link->sata_spd. This causes > problems with adapters/drives combination failing to establish a link > speed during probe autonegotiation. One exampe reported of this problem s/exampe/example/ > is an mvebu adapter with a 3Gbps port-multiplier box: autonegotiation > fails, leaving no recorded link speed and no rep@orted current link s/rep@orted/reported/ > speed. Probe retries also fail as no action is taken by sata_set_spd() > after each retry. > > Fix this by returning early in sata_down_spd_limit() only if we do have > a recorded link speed, that is, if link->sata_spd is not 0. With this > fix, a failed probe not leading to a recorded link speed is retried at > the lower 1.5 Gbps speed, with the link speed potentially increased > later on the second revalidate of the device if the device reports > that it supports higher link speeds. > > Reported-by: Marius Dinu <marius@psihoexpert.ro> > Fixes: 2dc0b46b5ea3 ("libata: sata_down_spd_limit should return if driver has not recorded sstatus speed") IIRC, checkpatch.pl with the default options should put the author on the commit being fixed on CC. CC:ing the original author is a good practice IMHO. > Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com> > --- > drivers/ata/libata-core.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c > index 884ae73b11ea..2ea572628b1c 100644 > --- a/drivers/ata/libata-core.c > +++ b/drivers/ata/libata-core.c > @@ -3109,7 +3109,7 @@ int sata_down_spd_limit(struct ata_link *link, u32 spd_limit) > */ > if (spd > 1) > mask &= (1 << (spd - 1)) - 1; > - else > + else if (link->sata_spd) > return -EINVAL; > > /* were we already at the bottom? */ > -- > 2.39.1 > With typos fixed: Reviewed-by: Niklas Cassel <niklas.cassel@wdc.com>
On 1/30/23 20:47, Niklas Cassel wrote: > On Mon, Jan 30, 2023 at 08:07:21PM +0900, Damien Le Moal wrote: >> Commit 2dc0b46b5ea3 ("libata: sata_down_spd_limit should return if >> driver has not recorded sstatus speed") changed the behavior of >> sata_down_spd_limit() to return doing nothing if a drive does not report >> a current link speed, to avoid reducing the link speed to the lowest 1.5 >> Gbps speed. >> >> However, the change assumed that a speed was recorded before probing >> (e.g. before a suspend/resume) and set in link->sata_spd. This causes >> problems with adapters/drives combination failing to establish a link >> speed during probe autonegotiation. One exampe reported of this problem > > s/exampe/example/ > >> is an mvebu adapter with a 3Gbps port-multiplier box: autonegotiation >> fails, leaving no recorded link speed and no rep@orted current link > > s/rep@orted/reported/ > >> speed. Probe retries also fail as no action is taken by sata_set_spd() >> after each retry. >> >> Fix this by returning early in sata_down_spd_limit() only if we do have >> a recorded link speed, that is, if link->sata_spd is not 0. With this >> fix, a failed probe not leading to a recorded link speed is retried at >> the lower 1.5 Gbps speed, with the link speed potentially increased >> later on the second revalidate of the device if the device reports >> that it supports higher link speeds. >> >> Reported-by: Marius Dinu <marius@psihoexpert.ro> >> Fixes: 2dc0b46b5ea3 ("libata: sata_down_spd_limit should return if driver has not recorded sstatus speed") > > IIRC, checkpatch.pl with the default options should put the author on the > commit being fixed on CC. CC:ing the original author is a good practice IMHO. Funny that I did run checkpatch and it did not complain about the above typos :) Thanks for the review, will fix this. > >> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com> >> --- >> drivers/ata/libata-core.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c >> index 884ae73b11ea..2ea572628b1c 100644 >> --- a/drivers/ata/libata-core.c >> +++ b/drivers/ata/libata-core.c >> @@ -3109,7 +3109,7 @@ int sata_down_spd_limit(struct ata_link *link, u32 spd_limit) >> */ >> if (spd > 1) >> mask &= (1 << (spd - 1)) - 1; >> - else >> + else if (link->sata_spd) >> return -EINVAL; >> >> /* were we already at the bottom? */ >> -- >> 2.39.1 >> > > With typos fixed: > Reviewed-by: Niklas Cassel <niklas.cassel@wdc.com>
On Mon, Jan 30, 2023 at 08:56:45PM +0900, Damien Le Moal wrote: > On 1/30/23 20:47, Niklas Cassel wrote: > > On Mon, Jan 30, 2023 at 08:07:21PM +0900, Damien Le Moal wrote: > >> Commit 2dc0b46b5ea3 ("libata: sata_down_spd_limit should return if > >> driver has not recorded sstatus speed") changed the behavior of > >> sata_down_spd_limit() to return doing nothing if a drive does not report > >> a current link speed, to avoid reducing the link speed to the lowest 1.5 > >> Gbps speed. > >> > >> However, the change assumed that a speed was recorded before probing > >> (e.g. before a suspend/resume) and set in link->sata_spd. This causes > >> problems with adapters/drives combination failing to establish a link > >> speed during probe autonegotiation. One exampe reported of this problem > > > > s/exampe/example/ > > > >> is an mvebu adapter with a 3Gbps port-multiplier box: autonegotiation > >> fails, leaving no recorded link speed and no rep@orted current link > > > > s/rep@orted/reported/ > > > >> speed. Probe retries also fail as no action is taken by sata_set_spd() > >> after each retry. > >> > >> Fix this by returning early in sata_down_spd_limit() only if we do have > >> a recorded link speed, that is, if link->sata_spd is not 0. With this > >> fix, a failed probe not leading to a recorded link speed is retried at > >> the lower 1.5 Gbps speed, with the link speed potentially increased > >> later on the second revalidate of the device if the device reports > >> that it supports higher link speeds. > >> > >> Reported-by: Marius Dinu <marius@psihoexpert.ro> > >> Fixes: 2dc0b46b5ea3 ("libata: sata_down_spd_limit should return if driver has not recorded sstatus speed") > > > > IIRC, checkpatch.pl with the default options should put the author on the > > commit being fixed on CC. CC:ing the original author is a good practice IMHO. > > Funny that I did run checkpatch and it did not complain about the above typos :) > Thanks for the review, will fix this. Sorry, I meant that get_maintainer.pl with the default options usually returns the author of the commit being fixed (see "blamed_fixes"): $ ./scripts/get_maintainer.pl 0001-ata-libata-Fix-sata_down_spd_limit-when-no-link-spee.patch Damien Le Moal <damien.lemoal@opensource.wdc.com> (maintainer:LIBATA SUBSYSTEM (Serial and Parallel ATA drivers)) Tejun Heo <tj@kernel.org> (blamed_fixes:1/1=100%) David Milburn <dmilburn@redhat.com> (blamed_fixes:1/1=100%) linux-ide@vger.kernel.org (open list:LIBATA SUBSYSTEM (Serial and Parallel ATA drivers)) linux-kernel@vger.kernel.org (open list) Kind regards, Niklas
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c index 884ae73b11ea..2ea572628b1c 100644 --- a/drivers/ata/libata-core.c +++ b/drivers/ata/libata-core.c @@ -3109,7 +3109,7 @@ int sata_down_spd_limit(struct ata_link *link, u32 spd_limit) */ if (spd > 1) mask &= (1 << (spd - 1)) - 1; - else + else if (link->sata_spd) return -EINVAL; /* were we already at the bottom? */
Commit 2dc0b46b5ea3 ("libata: sata_down_spd_limit should return if driver has not recorded sstatus speed") changed the behavior of sata_down_spd_limit() to return doing nothing if a drive does not report a current link speed, to avoid reducing the link speed to the lowest 1.5 Gbps speed. However, the change assumed that a speed was recorded before probing (e.g. before a suspend/resume) and set in link->sata_spd. This causes problems with adapters/drives combination failing to establish a link speed during probe autonegotiation. One exampe reported of this problem is an mvebu adapter with a 3Gbps port-multiplier box: autonegotiation fails, leaving no recorded link speed and no rep@orted current link speed. Probe retries also fail as no action is taken by sata_set_spd() after each retry. Fix this by returning early in sata_down_spd_limit() only if we do have a recorded link speed, that is, if link->sata_spd is not 0. With this fix, a failed probe not leading to a recorded link speed is retried at the lower 1.5 Gbps speed, with the link speed potentially increased later on the second revalidate of the device if the device reports that it supports higher link speeds. Reported-by: Marius Dinu <marius@psihoexpert.ro> Fixes: 2dc0b46b5ea3 ("libata: sata_down_spd_limit should return if driver has not recorded sstatus speed") Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com> --- drivers/ata/libata-core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)