Re: [PATCH v7 4/7] scsi: mvsas: Add libsas SATA sysfs attributes group

[Niklas Cassel <cassel@kernel.org>]

On Wed, Mar 06, 2024 at 01:13:22PM -0800, Igor Pylypiv wrote:
> On Wed, Mar 06, 2024 at 11:55:19AM +0100, Niklas Cassel wrote:
> > On Tue, Mar 05, 2024 at 05:22:23PM -0800, Igor Pylypiv wrote:
> > > The added sysfs attributes group enables the configuration of NCQ Priority
> > > feature for HBAs that rely on libsas to manage SATA devices.
> > > 
> > > Reviewed-by: John Garry <john.g.garry@oracle.com>
> > > Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
> > > Reviewed-by: Jason Yan <yanaijie@huawei.com>
> > > Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
> > > ---
> > >  drivers/scsi/mvsas/mv_init.c | 7 +++++++
> > >  1 file changed, 7 insertions(+)
> > > 
> > > diff --git a/drivers/scsi/mvsas/mv_init.c b/drivers/scsi/mvsas/mv_init.c
> > > index 43ebb331e216..f1090bb5f2c9 100644
> > > --- a/drivers/scsi/mvsas/mv_init.c
> > > +++ b/drivers/scsi/mvsas/mv_init.c
> > > @@ -26,6 +26,7 @@ static const struct mvs_chip_info mvs_chips[] = {
> > >  };
> > >  
> > >  static const struct attribute_group *mvst_host_groups[];
> > > +static const struct attribute_group *mvst_sdev_groups[];
> > 
> > I think you can remove this line.
> >
> I kept the forward declaration to match the mvst_host_groups style.
> 
> Perhaps mvs_sht can be moved to the bottom of the file so that all forward
> declarations can be removed? This can be done in a separate cleanup patch
> series.
> 
> I'll keep this and aic94xx patches as-is, unless there are objections.

Usually, you first do the cleanup, then you do your changes.
(That way, there are fewer lines changed, since each patch is smaller.)

But no objection from me.


Kind regards,
Niklas


> 
> > 
> > >  
> > >  #define SOC_SAS_NUM 2
> > >  
> > > @@ -53,6 +54,7 @@ static const struct scsi_host_template mvs_sht = {
> > >  	.compat_ioctl		= sas_ioctl,
> > >  #endif
> > >  	.shost_groups		= mvst_host_groups,
> > > +	.sdev_groups		= mvst_sdev_groups,
> > >  	.track_queue_depth	= 1,
> > >  };
> > >  
> > > @@ -779,6 +781,11 @@ static struct attribute *mvst_host_attrs[] = {
> > >  
> > >  ATTRIBUTE_GROUPS(mvst_host);
> > >  
> > > +static const struct attribute_group *mvst_sdev_groups[] = {
> > > +	&sas_ata_sdev_attr_group,
> > > +	NULL
> > > +};
> > 
> > ..and move these lines up to be after:
> > static const struct attribute_group *mvst_host_groups[];
> > 
> > 
> > > +
> > >  module_init(mvs_init);
> > >  module_exit(mvs_exit);
> > >  
> > > -- 
> > > 2.44.0.278.ge034bb2e1d-goog
> > >