Re: [PATCH v3 8/9] ata,scsi: Remove useless ata_sas_port_alloc() wrapper

[John Garry <john.g.garry@xxxxxxxxxx>]

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.

> 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.

>   		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;

> +	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);