On 2021/12/09 1:31, Hannes Reinecke wrote:
> Add an ata_port_classify() helper to print out the results from
> the device classification and remove the debugging statements
> from ata_dev_classify(). Also provide a mapping ata_dev_class_string()
> to provide a string representation for those instances calling
> ata_dev_classify() directly.
>
> Signed-off-by: Hannes Reinecke <hare@xxxxxxx>
> Reviewed-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@xxxxxxxxxxx>
> ---
> drivers/ata/libahci.c | 2 +-
> drivers/ata/libata-core.c | 67 ++++++++++++++++++++++++++++---------
> drivers/ata/libata-sff.c | 2 +-
> drivers/ata/sata_fsl.c | 2 +-
> drivers/ata/sata_inic162x.c | 2 +-
> drivers/ata/sata_sil24.c | 2 +-
> include/linux/libata.h | 2 ++
> 7 files changed, 58 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
> index 8b651f55fc84..21dbcd551443 100644
> --- a/drivers/ata/libahci.c
> +++ b/drivers/ata/libahci.c
> @@ -1300,7 +1300,7 @@ unsigned int ahci_dev_classify(struct ata_port *ap)
> tf.lbal = (tmp >> 8) & 0xff;
> tf.nsect = (tmp) & 0xff;
>
> - return ata_dev_classify(&tf);
> + return ata_port_classify(ap, &tf);
> }
> EXPORT_SYMBOL_GPL(ahci_dev_classify);
>
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index f6a9e2f22a94..6cac413caf83 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -969,6 +969,27 @@ const char *sata_spd_string(unsigned int spd)
> return spd_str[spd - 1];
> }
>
> +const char *ata_dev_class_string(unsigned int class)
This should be static.
> +{
> + static const char * const class_str[] = {
> + "unknown",
> + "ATA",
> + "ATA (unsupported)",
> + "ATAPI",
> + "ATAPI (unsupported",
> + "PMP",
> + "PMP (unsupported)",
> + "SEMB",
> + "SEMB (unsupported)",
> + "ZAC",
> + "ZAC (unsupported)",
> + "none",
> + };
Add a blank line after the declaration.
> + if (class == 0 || (class - 1) >= ARRAY_SIZE(class_str))
> + return "unknown";
> + return class_str[class - 1];
Why class - 1 ? The class_str entries directly match the values of the
ATA_DEV_XXX macros...
> +}
> +
> /**
> * ata_dev_classify - determine device type based on ATA-spec signature
> * @tf: ATA taskfile register set for device to be identified
> @@ -1007,36 +1028,50 @@ unsigned int ata_dev_classify(const struct ata_taskfile *tf)
> * SEMB signature. This is worked around in
> * ata_dev_read_id().
> */
> - if ((tf->lbam == 0) && (tf->lbah == 0)) {
> - DPRINTK("found ATA device by sig\n");
> + if ((tf->lbam == 0) && (tf->lbah == 0))
> return ATA_DEV_ATA;
> - }
>
> - if ((tf->lbam == 0x14) && (tf->lbah == 0xeb)) {
> - DPRINTK("found ATAPI device by sig\n");
> + if ((tf->lbam == 0x14) && (tf->lbah == 0xeb))
> return ATA_DEV_ATAPI;
> - }
>
> - if ((tf->lbam == 0x69) && (tf->lbah == 0x96)) {
> - DPRINTK("found PMP device by sig\n");
> + if ((tf->lbam == 0x69) && (tf->lbah == 0x96))
> return ATA_DEV_PMP;
> - }
>
> - if ((tf->lbam == 0x3c) && (tf->lbah == 0xc3)) {
> - DPRINTK("found SEMB device by sig (could be ATA device)\n");
> + if ((tf->lbam == 0x3c) && (tf->lbah == 0xc3))
> return ATA_DEV_SEMB;
> - }
>
> - if ((tf->lbam == 0xcd) && (tf->lbah == 0xab)) {
> - DPRINTK("found ZAC device by sig\n");
> + if ((tf->lbam == 0xcd) && (tf->lbah == 0xab))
> return ATA_DEV_ZAC;
> - }
>
> - DPRINTK("unknown device\n");
> return ATA_DEV_UNKNOWN;
> }
> EXPORT_SYMBOL_GPL(ata_dev_classify);
>
> +/**
> + * ata_port_classify - determine device type based on ATA-spec signature
> + * @ap: ATA port device on which the classification should be run
> + * @tf: ATA taskfile register set for device to be identified
> + *
> + * A wrapper around ata_dev_classify() to provide additional logging
> + *
> + * RETURNS:
> + * Device type, %ATA_DEV_ATA, %ATA_DEV_ATAPI, %ATA_DEV_PMP,
> + * %ATA_DEV_ZAC, or %ATA_DEV_UNKNOWN the event of failure.
> + */
> +unsigned int ata_port_classify(struct ata_port *ap,
> + const struct ata_taskfile *tf)
> +{
> + unsigned int class = ata_dev_classify(tf);
> +
> + if (class != ATA_DEV_UNKNOWN)
> + ata_port_dbg(ap, "found %s device by sig\n",
> + ata_dev_class_string(class));
> + else
> + ata_port_dbg(ap, "found unknown device\n");
Why the if-else here ? ata_dev_class_string(() handles ATA_DEV_UNKNOWN and "bad"
class values saying "unknown" already. So this all seems redundant. If this is
removed, then ata_dev_class_string() could be coded in here since this is the
sole user of that helper.
> + return class;
> +}
> +EXPORT_SYMBOL_GPL(ata_port_classify);
> +
> /**
> * ata_id_string - Convert IDENTIFY DEVICE page into string
> * @id: IDENTIFY DEVICE results we will examine
> diff --git a/drivers/ata/libata-sff.c b/drivers/ata/libata-sff.c
> index 39c026f3948c..a119fabe0919 100644
> --- a/drivers/ata/libata-sff.c
> +++ b/drivers/ata/libata-sff.c
> @@ -1853,7 +1853,7 @@ unsigned int ata_sff_dev_classify(struct ata_device *dev, int present,
> return ATA_DEV_NONE;
>
> /* determine if device is ATA or ATAPI */
> - class = ata_dev_classify(&tf);
> + class = ata_port_classify(ap, &tf);
>
> if (class == ATA_DEV_UNKNOWN) {
> /* If the device failed diagnostic, it's likely to
> diff --git a/drivers/ata/sata_fsl.c b/drivers/ata/sata_fsl.c
> index e5838b23c9e0..dbc57b7a58b8 100644
> --- a/drivers/ata/sata_fsl.c
> +++ b/drivers/ata/sata_fsl.c
> @@ -812,7 +812,7 @@ static unsigned int sata_fsl_dev_classify(struct ata_port *ap)
> tf.lbal = (temp >> 8) & 0xff;
> tf.nsect = temp & 0xff;
>
> - return ata_dev_classify(&tf);
> + return ata_port_classify(ap, &tf);
> }
>
> static int sata_fsl_hardreset(struct ata_link *link, unsigned int *class,
> diff --git a/drivers/ata/sata_inic162x.c b/drivers/ata/sata_inic162x.c
> index e517bd8822a5..b6239dae524a 100644
> --- a/drivers/ata/sata_inic162x.c
> +++ b/drivers/ata/sata_inic162x.c
> @@ -657,7 +657,7 @@ static int inic_hardreset(struct ata_link *link, unsigned int *class,
> }
>
> inic_tf_read(ap, &tf);
> - *class = ata_dev_classify(&tf);
> + *class = ata_port_classify(ap, &tf);
> }
>
> return 0;
> diff --git a/drivers/ata/sata_sil24.c b/drivers/ata/sata_sil24.c
> index f99ec6f7d7c0..7e9c1945dc81 100644
> --- a/drivers/ata/sata_sil24.c
> +++ b/drivers/ata/sata_sil24.c
> @@ -680,7 +680,7 @@ static int sil24_softreset(struct ata_link *link, unsigned int *class,
> }
>
> sil24_read_tf(ap, 0, &tf);
> - *class = ata_dev_classify(&tf);
> + *class = ata_port_classify(ap, &tf);
>
> DPRINTK("EXIT, class=%u\n", *class);
> return 0;
> diff --git a/include/linux/libata.h b/include/linux/libata.h
> index 2a8404b26083..235fdbeb19ea 100644
> --- a/include/linux/libata.h
> +++ b/include/linux/libata.h
> @@ -1160,6 +1160,8 @@ extern enum ata_completion_errors ata_noop_qc_prep(struct ata_queued_cmd *qc);
> extern void ata_sg_init(struct ata_queued_cmd *qc, struct scatterlist *sg,
> unsigned int n_elem);
> extern unsigned int ata_dev_classify(const struct ata_taskfile *tf);
> +extern unsigned int ata_port_classify(struct ata_port *ap,
> + const struct ata_taskfile *tf);
> extern void ata_dev_disable(struct ata_device *adev);
> extern void ata_id_string(const u16 *id, unsigned char *s,
> unsigned int ofs, unsigned int len);
>
--
Damien Le Moal
Western Digital Research
