| Message ID | 20210806074252.398482-5-damien.lemoal@wdc.com |
|---|---|
| State | New |
| Headers | show |
| Series | libata cleanups and improvements | expand |
On 8/6/21 9:42 AM, Damien Le Moal wrote: > Introduce the helper functions ata_dev_config_lba() and > ata_dev_config_chs() to configure the addressing capabilities of a > device. Each helper takes a string as argument for the addressing > information printed after these helpers execution completes. > > Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com> > --- > drivers/ata/libata-core.c | 110 ++++++++++++++++++++------------------ > 1 file changed, 59 insertions(+), 51 deletions(-) > > diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c > index b13194432e5a..2b6054cdd8fc 100644 > --- a/drivers/ata/libata-core.c > +++ b/drivers/ata/libata-core.c > @@ -2363,6 +2363,52 @@ static void ata_dev_config_trusted(struct ata_device *dev) > dev->flags |= ATA_DFLAG_TRUSTED; > } > > +static int ata_dev_config_lba(struct ata_device *dev, > + char *info, size_t infosz) > +{ > + const u16 *id = dev->id; > + int info_ofst; > + > + dev->flags |= ATA_DFLAG_LBA; > + > + if (ata_id_has_lba48(id)) { > + dev->flags |= ATA_DFLAG_LBA48; > + strscpy(info, "LBA48 ", infosz); > + > + if (dev->n_sectors >= (1UL << 28) && > + ata_id_has_flush_ext(id)) > + dev->flags |= ATA_DFLAG_FLUSH_EXT; > + } else { > + strscpy(info, "LBA ", infosz); > + } > + info_ofst = strlen(info); > + > + /* config NCQ */ > + return ata_dev_config_ncq(dev, info + info_ofst, > + infosz - info_ofst); > +} > + > +static void ata_dev_config_chs(struct ata_device *dev, > + char *info, size_t infosz) > +{ > + const u16 *id = dev->id; > + > + /* Default translation */ > + dev->cylinders = id[1]; > + dev->heads = id[3]; > + dev->sectors = id[6]; > + > + if (ata_id_current_chs_valid(id)) { > + /* Current CHS translation is valid. */ > + dev->cylinders = id[54]; > + dev->heads = id[55]; > + dev->sectors = id[56]; > + } > + > + snprintf(info, infosz, "CHS %u/%u/%u", > + dev->cylinders, dev->heads, dev->sectors); > +} > + > static void ata_dev_config_devslp(struct ata_device *dev) > { > u8 *sata_setting = dev->link->ap->sector_buf; > @@ -2418,6 +2464,7 @@ int ata_dev_configure(struct ata_device *dev) > char revbuf[7]; /* XYZ-99\0 */ > char fwrevbuf[ATA_ID_FW_REV_LEN+1]; > char modelbuf[ATA_ID_PROD_LEN+1]; > + char lba_info[40]; > int rc; > > if (!ata_dev_enabled(dev) && ata_msg_info(ap)) { > @@ -2539,61 +2586,22 @@ int ata_dev_configure(struct ata_device *dev) > } > > if (ata_id_has_lba(id)) { > - const char *lba_desc; > - char ncq_desc[24]; > - > - lba_desc = "LBA"; > - dev->flags |= ATA_DFLAG_LBA; > - if (ata_id_has_lba48(id)) { > - dev->flags |= ATA_DFLAG_LBA48; > - lba_desc = "LBA48"; > - > - if (dev->n_sectors >= (1UL << 28) && > - ata_id_has_flush_ext(id)) > - dev->flags |= ATA_DFLAG_FLUSH_EXT; > - } > - > - /* config NCQ */ > - rc = ata_dev_config_ncq(dev, ncq_desc, sizeof(ncq_desc)); > + rc = ata_dev_config_lba(dev, lba_info, sizeof(lba_info)); > if (rc) > return rc; > - > - /* print device info to dmesg */ > - if (ata_msg_drv(ap) && print_info) { > - ata_dev_info(dev, "%s: %s, %s, max %s\n", > - revbuf, modelbuf, fwrevbuf, > - ata_mode_string(xfer_mask)); > - ata_dev_info(dev, > - "%llu sectors, multi %u: %s %s\n", > - (unsigned long long)dev->n_sectors, > - dev->multi_count, lba_desc, ncq_desc); > - } > } else { > - /* CHS */ > - > - /* Default translation */ > - dev->cylinders = id[1]; > - dev->heads = id[3]; > - dev->sectors = id[6]; > - > - if (ata_id_current_chs_valid(id)) { > - /* Current CHS translation is valid. */ > - dev->cylinders = id[54]; > - dev->heads = id[55]; > - dev->sectors = id[56]; > - } > + ata_dev_config_chs(dev, lba_info, sizeof(lba_info)); > + } > > - /* print device info to dmesg */ > - if (ata_msg_drv(ap) && print_info) { > - ata_dev_info(dev, "%s: %s, %s, max %s\n", > - revbuf, modelbuf, fwrevbuf, > - ata_mode_string(xfer_mask)); > - ata_dev_info(dev, > - "%llu sectors, multi %u, CHS %u/%u/%u\n", > - (unsigned long long)dev->n_sectors, > - dev->multi_count, dev->cylinders, > - dev->heads, dev->sectors); > - } > + /* print device info to dmesg */ > + if (ata_msg_drv(ap) && print_info) { > + ata_dev_info(dev, "%s: %s, %s, max %s\n", > + revbuf, modelbuf, fwrevbuf, > + ata_mode_string(xfer_mask)); > + ata_dev_info(dev, > + "%llu sectors, multi %u, %s\n", > + (unsigned long long)dev->n_sectors, > + dev->multi_count, lba_info); > } > > ata_dev_config_devslp(dev); > Hmm. Can't say I like it. Can't you move the second 'ata_dev_info()' call into the respective functions, and kill the temporary buffer? Cheers, Hannes
On 2021/08/06 18:07, Hannes Reinecke wrote: > On 8/6/21 9:42 AM, Damien Le Moal wrote: >> Introduce the helper functions ata_dev_config_lba() and >> ata_dev_config_chs() to configure the addressing capabilities of a >> device. Each helper takes a string as argument for the addressing >> information printed after these helpers execution completes. >> >> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com> >> --- >> drivers/ata/libata-core.c | 110 ++++++++++++++++++++------------------ >> 1 file changed, 59 insertions(+), 51 deletions(-) >> >> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c >> index b13194432e5a..2b6054cdd8fc 100644 >> --- a/drivers/ata/libata-core.c >> +++ b/drivers/ata/libata-core.c >> @@ -2363,6 +2363,52 @@ static void ata_dev_config_trusted(struct ata_device *dev) >> dev->flags |= ATA_DFLAG_TRUSTED; >> } >> >> +static int ata_dev_config_lba(struct ata_device *dev, >> + char *info, size_t infosz) >> +{ >> + const u16 *id = dev->id; >> + int info_ofst; >> + >> + dev->flags |= ATA_DFLAG_LBA; >> + >> + if (ata_id_has_lba48(id)) { >> + dev->flags |= ATA_DFLAG_LBA48; >> + strscpy(info, "LBA48 ", infosz); >> + >> + if (dev->n_sectors >= (1UL << 28) && >> + ata_id_has_flush_ext(id)) >> + dev->flags |= ATA_DFLAG_FLUSH_EXT; >> + } else { >> + strscpy(info, "LBA ", infosz); >> + } >> + info_ofst = strlen(info); >> + >> + /* config NCQ */ >> + return ata_dev_config_ncq(dev, info + info_ofst, >> + infosz - info_ofst); >> +} >> + >> +static void ata_dev_config_chs(struct ata_device *dev, >> + char *info, size_t infosz) >> +{ >> + const u16 *id = dev->id; >> + >> + /* Default translation */ >> + dev->cylinders = id[1]; >> + dev->heads = id[3]; >> + dev->sectors = id[6]; >> + >> + if (ata_id_current_chs_valid(id)) { >> + /* Current CHS translation is valid. */ >> + dev->cylinders = id[54]; >> + dev->heads = id[55]; >> + dev->sectors = id[56]; >> + } >> + >> + snprintf(info, infosz, "CHS %u/%u/%u", >> + dev->cylinders, dev->heads, dev->sectors); >> +} >> + >> static void ata_dev_config_devslp(struct ata_device *dev) >> { >> u8 *sata_setting = dev->link->ap->sector_buf; >> @@ -2418,6 +2464,7 @@ int ata_dev_configure(struct ata_device *dev) >> char revbuf[7]; /* XYZ-99\0 */ >> char fwrevbuf[ATA_ID_FW_REV_LEN+1]; >> char modelbuf[ATA_ID_PROD_LEN+1]; >> + char lba_info[40]; >> int rc; >> >> if (!ata_dev_enabled(dev) && ata_msg_info(ap)) { >> @@ -2539,61 +2586,22 @@ int ata_dev_configure(struct ata_device *dev) >> } >> >> if (ata_id_has_lba(id)) { >> - const char *lba_desc; >> - char ncq_desc[24]; >> - >> - lba_desc = "LBA"; >> - dev->flags |= ATA_DFLAG_LBA; >> - if (ata_id_has_lba48(id)) { >> - dev->flags |= ATA_DFLAG_LBA48; >> - lba_desc = "LBA48"; >> - >> - if (dev->n_sectors >= (1UL << 28) && >> - ata_id_has_flush_ext(id)) >> - dev->flags |= ATA_DFLAG_FLUSH_EXT; >> - } >> - >> - /* config NCQ */ >> - rc = ata_dev_config_ncq(dev, ncq_desc, sizeof(ncq_desc)); >> + rc = ata_dev_config_lba(dev, lba_info, sizeof(lba_info)); >> if (rc) >> return rc; >> - >> - /* print device info to dmesg */ >> - if (ata_msg_drv(ap) && print_info) { >> - ata_dev_info(dev, "%s: %s, %s, max %s\n", >> - revbuf, modelbuf, fwrevbuf, >> - ata_mode_string(xfer_mask)); >> - ata_dev_info(dev, >> - "%llu sectors, multi %u: %s %s\n", >> - (unsigned long long)dev->n_sectors, >> - dev->multi_count, lba_desc, ncq_desc); >> - } >> } else { >> - /* CHS */ >> - >> - /* Default translation */ >> - dev->cylinders = id[1]; >> - dev->heads = id[3]; >> - dev->sectors = id[6]; >> - >> - if (ata_id_current_chs_valid(id)) { >> - /* Current CHS translation is valid. */ >> - dev->cylinders = id[54]; >> - dev->heads = id[55]; >> - dev->sectors = id[56]; >> - } >> + ata_dev_config_chs(dev, lba_info, sizeof(lba_info)); >> + } >> >> - /* print device info to dmesg */ >> - if (ata_msg_drv(ap) && print_info) { >> - ata_dev_info(dev, "%s: %s, %s, max %s\n", >> - revbuf, modelbuf, fwrevbuf, >> - ata_mode_string(xfer_mask)); >> - ata_dev_info(dev, >> - "%llu sectors, multi %u, CHS %u/%u/%u\n", >> - (unsigned long long)dev->n_sectors, >> - dev->multi_count, dev->cylinders, >> - dev->heads, dev->sectors); >> - } >> + /* print device info to dmesg */ >> + if (ata_msg_drv(ap) && print_info) { >> + ata_dev_info(dev, "%s: %s, %s, max %s\n", >> + revbuf, modelbuf, fwrevbuf, >> + ata_mode_string(xfer_mask)); >> + ata_dev_info(dev, >> + "%llu sectors, multi %u, %s\n", >> + (unsigned long long)dev->n_sectors, >> + dev->multi_count, lba_info); >> } >> >> ata_dev_config_devslp(dev); >> > Hmm. Can't say I like it. > Can't you move the second 'ata_dev_info()' call into the respective > functions, and kill the temporary buffer? That would reverse the order of the messages... And I wanted to avoid having an "if (ata_id_has_lba(id))" again just for the print. Moving the 2 ata_dev_info() calls into the respective functions was the other solution I tried, but then the functions require *a lot* more arguments (revbuf, modelbuf, fwrevbuf, ...) wich was not super nice. This one is the least ugly I thought... Any other idea ? > > Cheers, > > Hannes >
On 8/6/21 11:12 AM, Damien Le Moal wrote: > On 2021/08/06 18:07, Hannes Reinecke wrote: >> On 8/6/21 9:42 AM, Damien Le Moal wrote: >>> Introduce the helper functions ata_dev_config_lba() and >>> ata_dev_config_chs() to configure the addressing capabilities of a >>> device. Each helper takes a string as argument for the addressing >>> information printed after these helpers execution completes. >>> >>> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com> >>> --- >>> drivers/ata/libata-core.c | 110 ++++++++++++++++++++------------------ >>> 1 file changed, 59 insertions(+), 51 deletions(-) >>> >>> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c >>> index b13194432e5a..2b6054cdd8fc 100644 >>> --- a/drivers/ata/libata-core.c >>> +++ b/drivers/ata/libata-core.c [ .. ] >>> return rc; >>> - >>> - /* print device info to dmesg */ >>> - if (ata_msg_drv(ap) && print_info) { >>> - ata_dev_info(dev, "%s: %s, %s, max %s\n", >>> - revbuf, modelbuf, fwrevbuf, >>> - ata_mode_string(xfer_mask)); >>> - ata_dev_info(dev, >>> - "%llu sectors, multi %u: %s %s\n", >>> - (unsigned long long)dev->n_sectors, >>> - dev->multi_count, lba_desc, ncq_desc); >>> - } >>> } else { >>> - /* CHS */ >>> - >>> - /* Default translation */ >>> - dev->cylinders = id[1]; >>> - dev->heads = id[3]; >>> - dev->sectors = id[6]; >>> - >>> - if (ata_id_current_chs_valid(id)) { >>> - /* Current CHS translation is valid. */ >>> - dev->cylinders = id[54]; >>> - dev->heads = id[55]; >>> - dev->sectors = id[56]; >>> - } >>> + ata_dev_config_chs(dev, lba_info, sizeof(lba_info)); >>> + } >>> >>> - /* print device info to dmesg */ >>> - if (ata_msg_drv(ap) && print_info) { >>> - ata_dev_info(dev, "%s: %s, %s, max %s\n", >>> - revbuf, modelbuf, fwrevbuf, >>> - ata_mode_string(xfer_mask)); >>> - ata_dev_info(dev, >>> - "%llu sectors, multi %u, CHS %u/%u/%u\n", >>> - (unsigned long long)dev->n_sectors, >>> - dev->multi_count, dev->cylinders, >>> - dev->heads, dev->sectors); >>> - } >>> + /* print device info to dmesg */ >>> + if (ata_msg_drv(ap) && print_info) { >>> + ata_dev_info(dev, "%s: %s, %s, max %s\n", >>> + revbuf, modelbuf, fwrevbuf, >>> + ata_mode_string(xfer_mask)); >>> + ata_dev_info(dev, >>> + "%llu sectors, multi %u, %s\n", >>> + (unsigned long long)dev->n_sectors, >>> + dev->multi_count, lba_info); >>> } >>> >>> ata_dev_config_devslp(dev); >>> >> Hmm. Can't say I like it. >> Can't you move the second 'ata_dev_info()' call into the respective >> functions, and kill the temporary buffer? > > That would reverse the order of the messages... And I wanted to avoid having an > "if (ata_id_has_lba(id))" again just for the print. Moving the 2 ata_dev_info() > calls into the respective functions was the other solution I tried, but then the > functions require *a lot* more arguments (revbuf, modelbuf, fwrevbuf, ...) wich > was not super nice. > > This one is the least ugly I thought... Any other idea ? > Well, it should be possible to move the first 'ata_dev_info()' call _prior_ to the if(ata_id_has_lba(id)) condition, and then we can move the second call into the respective functions, no? Cheers, Hannes
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c index b13194432e5a..2b6054cdd8fc 100644 --- a/drivers/ata/libata-core.c +++ b/drivers/ata/libata-core.c @@ -2363,6 +2363,52 @@ static void ata_dev_config_trusted(struct ata_device *dev) dev->flags |= ATA_DFLAG_TRUSTED; } +static int ata_dev_config_lba(struct ata_device *dev, + char *info, size_t infosz) +{ + const u16 *id = dev->id; + int info_ofst; + + dev->flags |= ATA_DFLAG_LBA; + + if (ata_id_has_lba48(id)) { + dev->flags |= ATA_DFLAG_LBA48; + strscpy(info, "LBA48 ", infosz); + + if (dev->n_sectors >= (1UL << 28) && + ata_id_has_flush_ext(id)) + dev->flags |= ATA_DFLAG_FLUSH_EXT; + } else { + strscpy(info, "LBA ", infosz); + } + info_ofst = strlen(info); + + /* config NCQ */ + return ata_dev_config_ncq(dev, info + info_ofst, + infosz - info_ofst); +} + +static void ata_dev_config_chs(struct ata_device *dev, + char *info, size_t infosz) +{ + const u16 *id = dev->id; + + /* Default translation */ + dev->cylinders = id[1]; + dev->heads = id[3]; + dev->sectors = id[6]; + + if (ata_id_current_chs_valid(id)) { + /* Current CHS translation is valid. */ + dev->cylinders = id[54]; + dev->heads = id[55]; + dev->sectors = id[56]; + } + + snprintf(info, infosz, "CHS %u/%u/%u", + dev->cylinders, dev->heads, dev->sectors); +} + static void ata_dev_config_devslp(struct ata_device *dev) { u8 *sata_setting = dev->link->ap->sector_buf; @@ -2418,6 +2464,7 @@ int ata_dev_configure(struct ata_device *dev) char revbuf[7]; /* XYZ-99\0 */ char fwrevbuf[ATA_ID_FW_REV_LEN+1]; char modelbuf[ATA_ID_PROD_LEN+1]; + char lba_info[40]; int rc; if (!ata_dev_enabled(dev) && ata_msg_info(ap)) { @@ -2539,61 +2586,22 @@ int ata_dev_configure(struct ata_device *dev) } if (ata_id_has_lba(id)) { - const char *lba_desc; - char ncq_desc[24]; - - lba_desc = "LBA"; - dev->flags |= ATA_DFLAG_LBA; - if (ata_id_has_lba48(id)) { - dev->flags |= ATA_DFLAG_LBA48; - lba_desc = "LBA48"; - - if (dev->n_sectors >= (1UL << 28) && - ata_id_has_flush_ext(id)) - dev->flags |= ATA_DFLAG_FLUSH_EXT; - } - - /* config NCQ */ - rc = ata_dev_config_ncq(dev, ncq_desc, sizeof(ncq_desc)); + rc = ata_dev_config_lba(dev, lba_info, sizeof(lba_info)); if (rc) return rc; - - /* print device info to dmesg */ - if (ata_msg_drv(ap) && print_info) { - ata_dev_info(dev, "%s: %s, %s, max %s\n", - revbuf, modelbuf, fwrevbuf, - ata_mode_string(xfer_mask)); - ata_dev_info(dev, - "%llu sectors, multi %u: %s %s\n", - (unsigned long long)dev->n_sectors, - dev->multi_count, lba_desc, ncq_desc); - } } else { - /* CHS */ - - /* Default translation */ - dev->cylinders = id[1]; - dev->heads = id[3]; - dev->sectors = id[6]; - - if (ata_id_current_chs_valid(id)) { - /* Current CHS translation is valid. */ - dev->cylinders = id[54]; - dev->heads = id[55]; - dev->sectors = id[56]; - } + ata_dev_config_chs(dev, lba_info, sizeof(lba_info)); + } - /* print device info to dmesg */ - if (ata_msg_drv(ap) && print_info) { - ata_dev_info(dev, "%s: %s, %s, max %s\n", - revbuf, modelbuf, fwrevbuf, - ata_mode_string(xfer_mask)); - ata_dev_info(dev, - "%llu sectors, multi %u, CHS %u/%u/%u\n", - (unsigned long long)dev->n_sectors, - dev->multi_count, dev->cylinders, - dev->heads, dev->sectors); - } + /* print device info to dmesg */ + if (ata_msg_drv(ap) && print_info) { + ata_dev_info(dev, "%s: %s, %s, max %s\n", + revbuf, modelbuf, fwrevbuf, + ata_mode_string(xfer_mask)); + ata_dev_info(dev, + "%llu sectors, multi %u, %s\n", + (unsigned long long)dev->n_sectors, + dev->multi_count, lba_info); } ata_dev_config_devslp(dev);
Introduce the helper functions ata_dev_config_lba() and ata_dev_config_chs() to configure the addressing capabilities of a device. Each helper takes a string as argument for the addressing information printed after these helpers execution completes. Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com> --- drivers/ata/libata-core.c | 110 ++++++++++++++++++++------------------ 1 file changed, 59 insertions(+), 51 deletions(-)