| Message ID | 20221204081643.3835966-2-yanaijie@huawei.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | scsi: libsas: Some coding style fixes and cleanups | expand |
On 04/12/2022 08:16, Jason Yan wrote: > There is a sas_get_ata_command_set() declaration above sas_get_ata_info() > to make it compile ok. However this function is defined in the same file > below. So move it up to save the declaration. > > Cc: John Garry <john.g.garry@oracle.com> > Signed-off-by: Jason Yan <yanaijie@huawei.com> Apart from comments, below: Reviewed-by: John Garry <john.g.garry@oracle.com> > --- > drivers/scsi/libsas/sas_ata.c | 28 +++++++++++++--------------- > 1 file changed, 13 insertions(+), 15 deletions(-) > > diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c > index f7439bf9cdc6..34009c330eb2 100644 > --- a/drivers/scsi/libsas/sas_ata.c > +++ b/drivers/scsi/libsas/sas_ata.c > @@ -239,7 +239,19 @@ static struct sas_internal *dev_to_sas_internal(struct domain_device *dev) > return to_sas_internal(dev->port->ha->core.shost->transportt); > } > > -static int sas_get_ata_command_set(struct domain_device *dev); > +static int sas_get_ata_command_set(struct domain_device *dev) > +{ > + struct dev_to_host_fis *fis = > + (struct dev_to_host_fis *) dev->frame_rcvd; nit: I did not think that we add a whitespace before casting. And I think that it would be neater if fis was assigned separately, avoiding overflowing the line in this way. Having said that, it does seem odd to cast from u8 [] to struct dev_to_host_fis * and then back to (u8 *) in the ata_tf_from_fis() call, below. > + struct ata_taskfile tf; > + > + if (dev->dev_type == SAS_SATA_PENDING) > + return ATA_DEV_UNKNOWN; > + > + ata_tf_from_fis((const u8 *)fis, &tf); > + > + return ata_dev_classify(&tf); > +} > > int sas_get_ata_info(struct domain_device *dev, struct ex_phy *phy) > { > @@ -637,20 +649,6 @@ void sas_ata_task_abort(struct sas_task *task) > complete(waiting); > } > > -static int sas_get_ata_command_set(struct domain_device *dev) > -{ > - struct dev_to_host_fis *fis = > - (struct dev_to_host_fis *) dev->frame_rcvd; > - struct ata_taskfile tf; > - > - if (dev->dev_type == SAS_SATA_PENDING) > - return ATA_DEV_UNKNOWN; > - > - ata_tf_from_fis((const u8 *)fis, &tf); > - > - return ata_dev_classify(&tf); > -} > - > void sas_probe_sata(struct asd_sas_port *port) > { > struct domain_device *dev, *n;
On 2022/12/5 16:45, John Garry wrote: > On 04/12/2022 08:16, Jason Yan wrote: >> There is a sas_get_ata_command_set() declaration above sas_get_ata_info() >> to make it compile ok. However this function is defined in the same file >> below. So move it up to save the declaration. >> >> Cc: John Garry <john.g.garry@oracle.com> >> Signed-off-by: Jason Yan <yanaijie@huawei.com> > > Apart from comments, below: > Reviewed-by: John Garry <john.g.garry@oracle.com> > Thank you John. >> --- >> drivers/scsi/libsas/sas_ata.c | 28 +++++++++++++--------------- >> 1 file changed, 13 insertions(+), 15 deletions(-) >> >> diff --git a/drivers/scsi/libsas/sas_ata.c >> b/drivers/scsi/libsas/sas_ata.c >> index f7439bf9cdc6..34009c330eb2 100644 >> --- a/drivers/scsi/libsas/sas_ata.c >> +++ b/drivers/scsi/libsas/sas_ata.c >> @@ -239,7 +239,19 @@ static struct sas_internal >> *dev_to_sas_internal(struct domain_device *dev) >> return to_sas_internal(dev->port->ha->core.shost->transportt); >> } >> -static int sas_get_ata_command_set(struct domain_device *dev); >> +static int sas_get_ata_command_set(struct domain_device *dev) >> +{ >> + struct dev_to_host_fis *fis = >> + (struct dev_to_host_fis *) dev->frame_rcvd; > > nit: I did not think that we add a whitespace before casting. And I > think that it would be neater if fis was assigned separately, avoiding > overflowing the line in this way. > > Having said that, it does seem odd to cast from u8 [] to struct > dev_to_host_fis * and then back to (u8 *) in the ata_tf_from_fis() call, > below. Yeah, odd. I'd prefer remove 'fis' and then do: ata_tf_from_fis(dev->frame_rcvd, &tf). Thanks, Jason
diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c index f7439bf9cdc6..34009c330eb2 100644 --- a/drivers/scsi/libsas/sas_ata.c +++ b/drivers/scsi/libsas/sas_ata.c @@ -239,7 +239,19 @@ static struct sas_internal *dev_to_sas_internal(struct domain_device *dev) return to_sas_internal(dev->port->ha->core.shost->transportt); } -static int sas_get_ata_command_set(struct domain_device *dev); +static int sas_get_ata_command_set(struct domain_device *dev) +{ + struct dev_to_host_fis *fis = + (struct dev_to_host_fis *) dev->frame_rcvd; + struct ata_taskfile tf; + + if (dev->dev_type == SAS_SATA_PENDING) + return ATA_DEV_UNKNOWN; + + ata_tf_from_fis((const u8 *)fis, &tf); + + return ata_dev_classify(&tf); +} int sas_get_ata_info(struct domain_device *dev, struct ex_phy *phy) { @@ -637,20 +649,6 @@ void sas_ata_task_abort(struct sas_task *task) complete(waiting); } -static int sas_get_ata_command_set(struct domain_device *dev) -{ - struct dev_to_host_fis *fis = - (struct dev_to_host_fis *) dev->frame_rcvd; - struct ata_taskfile tf; - - if (dev->dev_type == SAS_SATA_PENDING) - return ATA_DEV_UNKNOWN; - - ata_tf_from_fis((const u8 *)fis, &tf); - - return ata_dev_classify(&tf); -} - void sas_probe_sata(struct asd_sas_port *port) { struct domain_device *dev, *n;
There is a sas_get_ata_command_set() declaration above sas_get_ata_info() to make it compile ok. However this function is defined in the same file below. So move it up to save the declaration. Cc: John Garry <john.g.garry@oracle.com> Signed-off-by: Jason Yan <yanaijie@huawei.com> --- drivers/scsi/libsas/sas_ata.c | 28 +++++++++++++--------------- 1 file changed, 13 insertions(+), 15 deletions(-)