Re: Transport Attributes -- attempt#3

[Martin Hicks <mort@bork.org>]

On Fri, Jan 16, 2004 at 02:54:57PM +0000, Christoph Hellwig wrote:
> > +menu "SCSI Transport Attributes (EXPERIMENTAL)"
> > +	depends on EXPERIMENTAL && SCSI!=n
> 
> Do you really need the experimental flags? Actually this option is
> rather bogus anyway - with your patches posted the qlogic drivers won't
> compile anymore without those set.  So either make it unconditional or
> let the drives select it.

Nope.  I just turned it on while I was testing stuff.  I will remove it
for the next patch, since a few people have tried it out now :-)

> 
> > +ifdef CONFIG_SCSI_SPI_ATTRS
> > +transport-objs			+= scsi_transport_spi.o
> > +endif
> > +ifdef CONFIG_SCSI_FC_ATTRS
> > +transport-objs			+= scsi_transport_fc.o
> > +endif
> > +
> >  scsi_mod-y			+= scsi.o hosts.o scsi_ioctl.o constants.o \
> >  				   scsicam.o scsi_error.o scsi_lib.o \
> >  				   scsi_scan.o scsi_syms.o scsi_sysfs.o \
> > -				   scsi_devinfo.o
> > +				   scsi_devinfo.o $(transport-objs)
> 
> This still looks quite overcomplicated, what about a simple:
> 
> scsi_mod-$(CONFIG_SCSI_SPI_ATTRS) += scsi_transport_spi.o
> scsi_mod-$(CONFIG_SCSI_FC_ATTRS)  += scsi_transport_fc.o

Okay.  I wasn't sure how to do that.  My Makefile foo is lacking.

Thanks.


> > +/* A blank transport template that is used in drivers that don't
> > + * yet implement Transport Attributes */
> > +struct scsi_transport_template blank_transport_template = { NULL };
> 
> The zero-initialization is not needed.

right.

> 
> > +void scsi_sysfs_cleanup_fc_transport(struct scsi_device *sdev)
> > +{
> > +	kfree(sdev->transport_attr_values);
> 
> I think you'll have to do the kfree in the device's ->release, but maybe
> I got all that sysfs-foo wrong :)

I'll take a closer look.

> > +	/* Default values for the transport attributes */
> > +	void *default_attr_values;
> 
> Should this really be a void *?

I think so.  It has to be a struct spi_transport_attrs or a
fc_transport_attrs, depending on which transport template we're talking
about.

> 
> > +extern struct scsi_transport_template blank_transport_template;
> 
> Should be in scsi_priv.h as it's not exported, no?

Yup.

Thanks again,
mh

-- 
Martin Hicks || mort@bork.org || PGP/GnuPG: 0x4C7F2BEE