| Message ID | 20220105153618.2395-1-pmenzel@molgen.mpg.de |
|---|---|
| State | New |
| Headers | show |
| Series | [v4,1/2] ahci: Rename flag `ATA_LFLAG_NO_DB_DELAY` to `ATA_LFLAG_NO_DEBOUNCE_DELAY` | expand |
On 1/6/22 00:36, Paul Menzel wrote: > The new name is longer, but clearer. It looks like you sent an incremental change on top of your previous patches. Please send a cleaned up series. One patch to introduce the ATA_LFLAG_NO_DEBOUNCE_DELAY flag & msleep bypass and another to add the AMD chipset. The commit message of this version patch 2 should be the explanation for the ATA_LFLAG_NO_DEBOUNCE_DELAY flag, not for adding a new chipset ID. > > Signed-off-by: Paul Menzel <pmenzel@molgen.mpg.de> > --- > drivers/ata/ahci_brcm.c | 2 +- > drivers/ata/libata-sata.c | 2 +- > include/linux/libata.h | 2 +- > 3 files changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/ata/ahci_brcm.c b/drivers/ata/ahci_brcm.c > index 6e9c5ade4c2ea..649815c196ed0 100644 > --- a/drivers/ata/ahci_brcm.c > +++ b/drivers/ata/ahci_brcm.c > @@ -333,7 +333,7 @@ static struct ata_port_operations ahci_brcm_platform_ops = { > > static const struct ata_port_info ahci_brcm_port_info = { > .flags = AHCI_FLAG_COMMON | ATA_FLAG_NO_DIPM, > - .link_flags = ATA_LFLAG_NO_DB_DELAY, > + .link_flags = ATA_LFLAG_NO_DEBOUNCE_DELAY, > .pio_mask = ATA_PIO4, > .udma_mask = ATA_UDMA6, > .port_ops = &ahci_brcm_platform_ops, > diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c > index b9c77885b8726..67b2e7cf3cc4e 100644 > --- a/drivers/ata/libata-sata.c > +++ b/drivers/ata/libata-sata.c > @@ -317,7 +317,7 @@ int sata_link_resume(struct ata_link *link, const unsigned long *params, > * immediately after resuming. Delay 200ms before > * debouncing. > */ > - if (!(link->flags & ATA_LFLAG_NO_DB_DELAY)) > + if (!(link->flags & ATA_LFLAG_NO_DEBOUNCE_DELAY)) > ata_msleep(link->ap, 200); > > /* is SControl restored correctly? */ > diff --git a/include/linux/libata.h b/include/linux/libata.h > index 2a8404b26083c..15802e644962d 100644 > --- a/include/linux/libata.h > +++ b/include/linux/libata.h > @@ -191,7 +191,7 @@ enum { > ATA_LFLAG_NO_LPM = (1 << 8), /* disable LPM on this link */ > ATA_LFLAG_RST_ONCE = (1 << 9), /* limit recovery to one reset */ > ATA_LFLAG_CHANGED = (1 << 10), /* LPM state changed on this link */ > - ATA_LFLAG_NO_DB_DELAY = (1 << 11), /* no debounce delay on link resume */ > + ATA_LFLAG_NO_DEBOUNCE_DELAY = (1 << 11), /* no debounce delay on link resume */ > > /* struct ata_port flags */ > ATA_FLAG_SLAVE_POSS = (1 << 0), /* host supports slave dev */
Dear Damien, Am 06.01.22 um 03:52 schrieb Damien Le Moal: > On 1/6/22 00:36, Paul Menzel wrote: >> The new name is longer, but clearer. > > It looks like you sent an incremental change on top of your previous > patches. Please send a cleaned up series. One patch to introduce the > ATA_LFLAG_NO_DEBOUNCE_DELAY flag & msleep bypass and another to add the > AMD chipset. > > The commit message of this version patch 2 should be the explanation for > the ATA_LFLAG_NO_DEBOUNCE_DELAY flag, not for adding a new chipset ID. Sorry, but I think you misread the patch. The flag `ATA_LFLAG_NO_DB_DELAY` exists since commit e39b2bb3b7 (libata: skip debounce delay on link resume). Kind regards, Paul >> Signed-off-by: Paul Menzel <pmenzel@molgen.mpg.de> >> --- >> drivers/ata/ahci_brcm.c | 2 +- >> drivers/ata/libata-sata.c | 2 +- >> include/linux/libata.h | 2 +- >> 3 files changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/ata/ahci_brcm.c b/drivers/ata/ahci_brcm.c >> index 6e9c5ade4c2ea..649815c196ed0 100644 >> --- a/drivers/ata/ahci_brcm.c >> +++ b/drivers/ata/ahci_brcm.c >> @@ -333,7 +333,7 @@ static struct ata_port_operations ahci_brcm_platform_ops = { >> >> static const struct ata_port_info ahci_brcm_port_info = { >> .flags = AHCI_FLAG_COMMON | ATA_FLAG_NO_DIPM, >> - .link_flags = ATA_LFLAG_NO_DB_DELAY, >> + .link_flags = ATA_LFLAG_NO_DEBOUNCE_DELAY, >> .pio_mask = ATA_PIO4, >> .udma_mask = ATA_UDMA6, >> .port_ops = &ahci_brcm_platform_ops, >> diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c >> index b9c77885b8726..67b2e7cf3cc4e 100644 >> --- a/drivers/ata/libata-sata.c >> +++ b/drivers/ata/libata-sata.c >> @@ -317,7 +317,7 @@ int sata_link_resume(struct ata_link *link, const unsigned long *params, >> * immediately after resuming. Delay 200ms before >> * debouncing. >> */ >> - if (!(link->flags & ATA_LFLAG_NO_DB_DELAY)) >> + if (!(link->flags & ATA_LFLAG_NO_DEBOUNCE_DELAY)) >> ata_msleep(link->ap, 200); >> >> /* is SControl restored correctly? */ >> diff --git a/include/linux/libata.h b/include/linux/libata.h >> index 2a8404b26083c..15802e644962d 100644 >> --- a/include/linux/libata.h >> +++ b/include/linux/libata.h >> @@ -191,7 +191,7 @@ enum { >> ATA_LFLAG_NO_LPM = (1 << 8), /* disable LPM on this link */ >> ATA_LFLAG_RST_ONCE = (1 << 9), /* limit recovery to one reset */ >> ATA_LFLAG_CHANGED = (1 << 10), /* LPM state changed on this link */ >> - ATA_LFLAG_NO_DB_DELAY = (1 << 11), /* no debounce delay on link resume */ >> + ATA_LFLAG_NO_DEBOUNCE_DELAY = (1 << 11), /* no debounce delay on link resume */ >> >> /* struct ata_port flags */ >> ATA_FLAG_SLAVE_POSS = (1 << 0), /* host supports slave dev */ > >
On 1/6/22 16:35, Paul Menzel wrote: > Dear Damien, > > > Am 06.01.22 um 03:52 schrieb Damien Le Moal: >> On 1/6/22 00:36, Paul Menzel wrote: >>> The new name is longer, but clearer. >> >> It looks like you sent an incremental change on top of your previous >> patches. Please send a cleaned up series. One patch to introduce the >> ATA_LFLAG_NO_DEBOUNCE_DELAY flag & msleep bypass and another to add the >> AMD chipset. >> >> The commit message of this version patch 2 should be the explanation for >> the ATA_LFLAG_NO_DEBOUNCE_DELAY flag, not for adding a new chipset ID. > > Sorry, but I think you misread the patch. The flag > `ATA_LFLAG_NO_DB_DELAY` exists since commit e39b2bb3b7 (libata: skip > debounce delay on link resume). Arg. Indeed. Ignore my comment :) Let's wait to see if there are any more comments and I will queue this up for 5.17. Going forward, it would be interesting to work on reversing the default: no delay by default and mark all old & unknown adapters as needing the delay. As you mentioned, most modern AHCI adapters most likely do not need it. Are you willing to take this and send a patches ? > > > Kind regards, > > Paul > > >>> Signed-off-by: Paul Menzel <pmenzel@molgen.mpg.de> >>> --- >>> drivers/ata/ahci_brcm.c | 2 +- >>> drivers/ata/libata-sata.c | 2 +- >>> include/linux/libata.h | 2 +- >>> 3 files changed, 3 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/ata/ahci_brcm.c b/drivers/ata/ahci_brcm.c >>> index 6e9c5ade4c2ea..649815c196ed0 100644 >>> --- a/drivers/ata/ahci_brcm.c >>> +++ b/drivers/ata/ahci_brcm.c >>> @@ -333,7 +333,7 @@ static struct ata_port_operations ahci_brcm_platform_ops = { >>> >>> static const struct ata_port_info ahci_brcm_port_info = { >>> .flags = AHCI_FLAG_COMMON | ATA_FLAG_NO_DIPM, >>> - .link_flags = ATA_LFLAG_NO_DB_DELAY, >>> + .link_flags = ATA_LFLAG_NO_DEBOUNCE_DELAY, >>> .pio_mask = ATA_PIO4, >>> .udma_mask = ATA_UDMA6, >>> .port_ops = &ahci_brcm_platform_ops, >>> diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c >>> index b9c77885b8726..67b2e7cf3cc4e 100644 >>> --- a/drivers/ata/libata-sata.c >>> +++ b/drivers/ata/libata-sata.c >>> @@ -317,7 +317,7 @@ int sata_link_resume(struct ata_link *link, const unsigned long *params, >>> * immediately after resuming. Delay 200ms before >>> * debouncing. >>> */ >>> - if (!(link->flags & ATA_LFLAG_NO_DB_DELAY)) >>> + if (!(link->flags & ATA_LFLAG_NO_DEBOUNCE_DELAY)) >>> ata_msleep(link->ap, 200); >>> >>> /* is SControl restored correctly? */ >>> diff --git a/include/linux/libata.h b/include/linux/libata.h >>> index 2a8404b26083c..15802e644962d 100644 >>> --- a/include/linux/libata.h >>> +++ b/include/linux/libata.h >>> @@ -191,7 +191,7 @@ enum { >>> ATA_LFLAG_NO_LPM = (1 << 8), /* disable LPM on this link */ >>> ATA_LFLAG_RST_ONCE = (1 << 9), /* limit recovery to one reset */ >>> ATA_LFLAG_CHANGED = (1 << 10), /* LPM state changed on this link */ >>> - ATA_LFLAG_NO_DB_DELAY = (1 << 11), /* no debounce delay on link resume */ >>> + ATA_LFLAG_NO_DEBOUNCE_DELAY = (1 << 11), /* no debounce delay on link resume */ >>> >>> /* struct ata_port flags */ >>> ATA_FLAG_SLAVE_POSS = (1 << 0), /* host supports slave dev */ >> >>
Dear Damien, Am 06.01.22 um 08:55 schrieb Damien Le Moal: > On 1/6/22 16:35, Paul Menzel wrote: >> Am 06.01.22 um 03:52 schrieb Damien Le Moal: >>> On 1/6/22 00:36, Paul Menzel wrote: >>>> The new name is longer, but clearer. >>> >>> It looks like you sent an incremental change on top of your previous >>> patches. Please send a cleaned up series. One patch to introduce the >>> ATA_LFLAG_NO_DEBOUNCE_DELAY flag & msleep bypass and another to add the >>> AMD chipset. >>> >>> The commit message of this version patch 2 should be the explanation for >>> the ATA_LFLAG_NO_DEBOUNCE_DELAY flag, not for adding a new chipset ID. >> >> Sorry, but I think you misread the patch. The flag >> `ATA_LFLAG_NO_DB_DELAY` exists since commit e39b2bb3b7 (libata: skip >> debounce delay on link resume). > > Arg. Indeed. Ignore my comment :) > > Let's wait to see if there are any more comments and I will queue this > up for 5.17. Thank you. In the second patch, I should have probably mentioned, that the flag already exists. Maybe you can amend that before applying them. > Going forward, it would be interesting to work on reversing the default: > no delay by default and mark all old & unknown adapters as needing the > delay. As you mentioned, most modern AHCI adapters most likely do not > need it. > > Are you willing to take this and send a patches ? Yes, I have it on my to-do list, but cannot promise anything. Kind regards, Paul >>>> Signed-off-by: Paul Menzel <pmenzel@molgen.mpg.de> >>>> --- >>>> drivers/ata/ahci_brcm.c | 2 +- >>>> drivers/ata/libata-sata.c | 2 +- >>>> include/linux/libata.h | 2 +- >>>> 3 files changed, 3 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/drivers/ata/ahci_brcm.c b/drivers/ata/ahci_brcm.c >>>> index 6e9c5ade4c2ea..649815c196ed0 100644 >>>> --- a/drivers/ata/ahci_brcm.c >>>> +++ b/drivers/ata/ahci_brcm.c >>>> @@ -333,7 +333,7 @@ static struct ata_port_operations ahci_brcm_platform_ops = { >>>> >>>> static const struct ata_port_info ahci_brcm_port_info = { >>>> .flags = AHCI_FLAG_COMMON | ATA_FLAG_NO_DIPM, >>>> - .link_flags = ATA_LFLAG_NO_DB_DELAY, >>>> + .link_flags = ATA_LFLAG_NO_DEBOUNCE_DELAY, >>>> .pio_mask = ATA_PIO4, >>>> .udma_mask = ATA_UDMA6, >>>> .port_ops = &ahci_brcm_platform_ops, >>>> diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c >>>> index b9c77885b8726..67b2e7cf3cc4e 100644 >>>> --- a/drivers/ata/libata-sata.c >>>> +++ b/drivers/ata/libata-sata.c >>>> @@ -317,7 +317,7 @@ int sata_link_resume(struct ata_link *link, const unsigned long *params, >>>> * immediately after resuming. Delay 200ms before >>>> * debouncing. >>>> */ >>>> - if (!(link->flags & ATA_LFLAG_NO_DB_DELAY)) >>>> + if (!(link->flags & ATA_LFLAG_NO_DEBOUNCE_DELAY)) >>>> ata_msleep(link->ap, 200); >>>> >>>> /* is SControl restored correctly? */ >>>> diff --git a/include/linux/libata.h b/include/linux/libata.h >>>> index 2a8404b26083c..15802e644962d 100644 >>>> --- a/include/linux/libata.h >>>> +++ b/include/linux/libata.h >>>> @@ -191,7 +191,7 @@ enum { >>>> ATA_LFLAG_NO_LPM = (1 << 8), /* disable LPM on this link */ >>>> ATA_LFLAG_RST_ONCE = (1 << 9), /* limit recovery to one reset */ >>>> ATA_LFLAG_CHANGED = (1 << 10), /* LPM state changed on this link */ >>>> - ATA_LFLAG_NO_DB_DELAY = (1 << 11), /* no debounce delay on link resume */ >>>> + ATA_LFLAG_NO_DEBOUNCE_DELAY = (1 << 11), /* no debounce delay on link resume */ >>>> >>>> /* struct ata_port flags */ >>>> ATA_FLAG_SLAVE_POSS = (1 << 0), /* host supports slave dev */ >>> >>>
On 1/6/22 00:36, Paul Menzel wrote: > The new name is longer, but clearer. > > Signed-off-by: Paul Menzel <pmenzel@molgen.mpg.de> > --- > drivers/ata/ahci_brcm.c | 2 +- > drivers/ata/libata-sata.c | 2 +- > include/linux/libata.h | 2 +- > 3 files changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/ata/ahci_brcm.c b/drivers/ata/ahci_brcm.c > index 6e9c5ade4c2ea..649815c196ed0 100644 > --- a/drivers/ata/ahci_brcm.c > +++ b/drivers/ata/ahci_brcm.c > @@ -333,7 +333,7 @@ static struct ata_port_operations ahci_brcm_platform_ops = { > > static const struct ata_port_info ahci_brcm_port_info = { > .flags = AHCI_FLAG_COMMON | ATA_FLAG_NO_DIPM, > - .link_flags = ATA_LFLAG_NO_DB_DELAY, > + .link_flags = ATA_LFLAG_NO_DEBOUNCE_DELAY, > .pio_mask = ATA_PIO4, > .udma_mask = ATA_UDMA6, > .port_ops = &ahci_brcm_platform_ops, > diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c > index b9c77885b8726..67b2e7cf3cc4e 100644 > --- a/drivers/ata/libata-sata.c > +++ b/drivers/ata/libata-sata.c > @@ -317,7 +317,7 @@ int sata_link_resume(struct ata_link *link, const unsigned long *params, > * immediately after resuming. Delay 200ms before > * debouncing. > */ > - if (!(link->flags & ATA_LFLAG_NO_DB_DELAY)) > + if (!(link->flags & ATA_LFLAG_NO_DEBOUNCE_DELAY)) > ata_msleep(link->ap, 200); > > /* is SControl restored correctly? */ > diff --git a/include/linux/libata.h b/include/linux/libata.h > index 2a8404b26083c..15802e644962d 100644 > --- a/include/linux/libata.h > +++ b/include/linux/libata.h > @@ -191,7 +191,7 @@ enum { > ATA_LFLAG_NO_LPM = (1 << 8), /* disable LPM on this link */ > ATA_LFLAG_RST_ONCE = (1 << 9), /* limit recovery to one reset */ > ATA_LFLAG_CHANGED = (1 << 10), /* LPM state changed on this link */ > - ATA_LFLAG_NO_DB_DELAY = (1 << 11), /* no debounce delay on link resume */ > + ATA_LFLAG_NO_DEBOUNCE_DELAY = (1 << 11), /* no debounce delay on link resume */ > > /* struct ata_port flags */ > ATA_FLAG_SLAVE_POSS = (1 << 0), /* host supports slave dev */ I applied both patches to for-5.17 (sorry, I am late, this fell through the cracks...). I simplified the commit messages though, especially patch 2, since the main point of that patch is to introduce support for a new controller rather than discussing the debounce delay thingy. If you are OK with it, I will reuse your well written commit message for a patch removing the debounce delay though :) Working on it now, I will post something by week end after testing.
diff --git a/drivers/ata/ahci_brcm.c b/drivers/ata/ahci_brcm.c index 6e9c5ade4c2ea..649815c196ed0 100644 --- a/drivers/ata/ahci_brcm.c +++ b/drivers/ata/ahci_brcm.c @@ -333,7 +333,7 @@ static struct ata_port_operations ahci_brcm_platform_ops = { static const struct ata_port_info ahci_brcm_port_info = { .flags = AHCI_FLAG_COMMON | ATA_FLAG_NO_DIPM, - .link_flags = ATA_LFLAG_NO_DB_DELAY, + .link_flags = ATA_LFLAG_NO_DEBOUNCE_DELAY, .pio_mask = ATA_PIO4, .udma_mask = ATA_UDMA6, .port_ops = &ahci_brcm_platform_ops, diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c index b9c77885b8726..67b2e7cf3cc4e 100644 --- a/drivers/ata/libata-sata.c +++ b/drivers/ata/libata-sata.c @@ -317,7 +317,7 @@ int sata_link_resume(struct ata_link *link, const unsigned long *params, * immediately after resuming. Delay 200ms before * debouncing. */ - if (!(link->flags & ATA_LFLAG_NO_DB_DELAY)) + if (!(link->flags & ATA_LFLAG_NO_DEBOUNCE_DELAY)) ata_msleep(link->ap, 200); /* is SControl restored correctly? */ diff --git a/include/linux/libata.h b/include/linux/libata.h index 2a8404b26083c..15802e644962d 100644 --- a/include/linux/libata.h +++ b/include/linux/libata.h @@ -191,7 +191,7 @@ enum { ATA_LFLAG_NO_LPM = (1 << 8), /* disable LPM on this link */ ATA_LFLAG_RST_ONCE = (1 << 9), /* limit recovery to one reset */ ATA_LFLAG_CHANGED = (1 << 10), /* LPM state changed on this link */ - ATA_LFLAG_NO_DB_DELAY = (1 << 11), /* no debounce delay on link resume */ + ATA_LFLAG_NO_DEBOUNCE_DELAY = (1 << 11), /* no debounce delay on link resume */ /* struct ata_port flags */ ATA_FLAG_SLAVE_POSS = (1 << 0), /* host supports slave dev */
The new name is longer, but clearer. Signed-off-by: Paul Menzel <pmenzel@molgen.mpg.de> --- drivers/ata/ahci_brcm.c | 2 +- drivers/ata/libata-sata.c | 2 +- include/linux/libata.h | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-)