On Wed, Jul 03, 2024 at 11:20:51AM +0100, John Garry wrote:
> On 02/07/2024 17:08, Niklas Cassel wrote:
> > Now when the ap->print_id assignment has moved to ata_port_alloc(),
> > we can remove the useless ata_sas_port_alloc() wrapper.
>
> nit: I don't know why you say it is useless. It is used today, so has some
> use, right?
>
> I'd be more inclined to say that it is only used in one location, so can be
> inlined there.
Sure, I will clarify the commit message.
(I will also clarify the commit message for the other commit that also
says 'remove useless wrappers'.)
>
> >
> > Reviewed-by: Hannes Reinecke <hare@xxxxxxx>
> > Signed-off-by: Niklas Cassel <cassel@xxxxxxxxxx>
> > ---
> > drivers/ata/libata-core.c | 1 +
> > drivers/ata/libata-sata.c | 35 -----------------------------------
> > drivers/ata/libata.h | 1 -
> > drivers/scsi/libsas/sas_ata.c | 10 ++++++++--
> > include/linux/libata.h | 3 +--
> > 5 files changed, 10 insertions(+), 40 deletions(-)
> >
> > diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> > index 5031064834be..22e7b09c93b1 100644
> > --- a/drivers/ata/libata-core.c
> > +++ b/drivers/ata/libata-core.c
> > @@ -5493,6 +5493,7 @@ struct ata_port *ata_port_alloc(struct ata_host *host)
> > return ap;
> > }
> > +EXPORT_SYMBOL_GPL(ata_port_alloc);
> > void ata_port_free(struct ata_port *ap)
> > {
> > diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
> > index b602247604dc..48660d445602 100644
> > --- a/drivers/ata/libata-sata.c
> > +++ b/drivers/ata/libata-sata.c
> > @@ -1204,41 +1204,6 @@ int ata_scsi_change_queue_depth(struct scsi_device *sdev, int queue_depth)
> > }
> > EXPORT_SYMBOL_GPL(ata_scsi_change_queue_depth);
> > -/**
> > - * ata_sas_port_alloc - Allocate port for a SAS attached SATA device
> > - * @host: ATA host container for all SAS ports
> > - * @port_info: Information from low-level host driver
> > - * @shost: SCSI host that the scsi device is attached to
> > - *
> > - * LOCKING:
> > - * PCI/etc. bus probe sem.
> > - *
> > - * RETURNS:
> > - * ata_port pointer on success / NULL on failure.
> > - */
> > -
> > -struct ata_port *ata_sas_port_alloc(struct ata_host *host,
> > - struct ata_port_info *port_info,
> > - struct Scsi_Host *shost)
> > -{
> > - struct ata_port *ap;
> > -
> > - ap = ata_port_alloc(host);
> > - if (!ap)
> > - return NULL;
> > -
> > - ap->port_no = 0;
> > - ap->pio_mask = port_info->pio_mask;
> > - ap->mwdma_mask = port_info->mwdma_mask;
> > - ap->udma_mask = port_info->udma_mask;
> > - ap->flags |= port_info->flags;
> > - ap->ops = port_info->port_ops;
> > - ap->cbl = ATA_CBL_SATA;
> > -
> > - return ap;
> > -}
> > -EXPORT_SYMBOL_GPL(ata_sas_port_alloc);
> > -
> > /**
> > * ata_sas_device_configure - Default device_configure routine for libata
> > * devices
> > diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
> > index 5ea194ae8a8b..6abf265f626e 100644
> > --- a/drivers/ata/libata.h
> > +++ b/drivers/ata/libata.h
> > @@ -81,7 +81,6 @@ extern void ata_link_init(struct ata_port *ap, struct ata_link *link, int pmp);
> > extern int sata_link_init_spd(struct ata_link *link);
> > extern int ata_task_ioctl(struct scsi_device *scsidev, void __user *arg);
> > extern int ata_cmd_ioctl(struct scsi_device *scsidev, void __user *arg);
> > -extern struct ata_port *ata_port_alloc(struct ata_host *host);
> > extern const char *sata_spd_string(unsigned int spd);
> > extern unsigned int ata_read_log_page(struct ata_device *dev, u8 log,
> > u8 page, void *buf, unsigned int sectors);
> > diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c
> > index ab4ddeea4909..80299f517081 100644
> > --- a/drivers/scsi/libsas/sas_ata.c
> > +++ b/drivers/scsi/libsas/sas_ata.c
> > @@ -597,13 +597,19 @@ int sas_ata_init(struct domain_device *found_dev)
> > ata_host_init(ata_host, ha->dev, &sas_sata_ops);
> > - ap = ata_sas_port_alloc(ata_host, &sata_port_info, shost);
> > + ap = ata_port_alloc(ata_host);
> > if (!ap) {
> > - pr_err("ata_sas_port_alloc failed.\n");
> > + pr_err("ata_port_alloc failed.\n");
>
> nit: Are these really useful prints? AFAICS, ata_port_alloc() can only fail
> due to ENOMEM and we generally don't print ENOMEM errors in drivers. I know
> that we change the error code, below, but still I doubt its value.
ata_port_alloc() can also fail if there are no free IDs (ida_alloc_min()
returns -ENOSPC), so I will leave the print for now.
>
> > rc = -ENODEV;
> > goto free_host;
> > }
> > + ap->port_no = 0;
> > + ap->pio_mask = sata_port_info.pio_mask;
>
> Why do we even have sata_port_info now, if we are not passing a complete
> structure? I mean, why not:
>
> ap->pio_mask = ATA_PI04;
Good point, I will remove the structure and perform the initialization
directly, like you are suggesting. This will remove even more lines :)
>
> > + ap->mwdma_mask = sata_port_info.mwdma_mask;
> > + ap->udma_mask = sata_port_info.udma_mask;
> > + ap->flags |= sata_port_info.flags;
> > + ap->ops = sata_port_info.port_ops;
> > ap->private_data = found_dev;
> > ap->cbl = ATA_CBL_SATA;
> > ap->scsi_host = shost;
> > diff --git a/include/linux/libata.h b/include/linux/libata.h
> > index 84a7bfbac9fa..17394098bee9 100644
> > --- a/include/linux/libata.h
> > +++ b/include/linux/libata.h
> > @@ -1244,9 +1244,8 @@ extern int sata_link_debounce(struct ata_link *link,
> > extern int sata_link_scr_lpm(struct ata_link *link, enum ata_lpm_policy policy,
> > bool spm_wakeup);
> > extern int ata_slave_link_init(struct ata_port *ap);
> > -extern struct ata_port *ata_sas_port_alloc(struct ata_host *,
> > - struct ata_port_info *, struct Scsi_Host *);
> > extern void ata_port_probe(struct ata_port *ap);
> > +extern struct ata_port *ata_port_alloc(struct ata_host *host);
> > extern void ata_port_free(struct ata_port *ap);
> > extern int ata_tport_add(struct device *parent, struct ata_port *ap);
> > extern void ata_tport_delete(struct ata_port *ap);
>
