| Message ID | 20240702160756.596955-19-cassel@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | ata,libsas: Assign the unique id used for printing earlier | expand |
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@suse.de> > > Signed-off-by: Niklas Cassel <cassel@kernel.org> > > --- > > 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); >
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"); rc = -ENODEV; goto free_host; } + ap->port_no = 0; + ap->pio_mask = sata_port_info.pio_mask; + 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);