Re: Transport Attributes -- attempt#3

[Christoph Hellwig <hch@infradead.org>]

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

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

> +#ifdef CONFIG_SCSI_SPI_ATTRS
> +EXPORT_SYMBOL(spi_transport_template);
> +#endif
> +#ifdef CONFIG_SCSI_FC_ATTRS
> +EXPORT_SYMBOL(fc_transport_template);
> +#endif

Should just move to the files declaring it.

> +	if (sdev->transport_classdev.class) {
> +		attrs = sdev->host->transportt->attrs;
> +		for (i = 0; attrs[i]; i++) {
> +			error = class_device_create_file(&sdev->transport_classdev,
> +							 attrs[i]);
> +			if (error)
> +				scsi_remove_device(sdev);

Missing break here, in fact the cleanup mioght better be after a lable
you goto to.

> +/* 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.

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

> +	void			* transport_attr_values;

	void			*transport_attr_values;

> +	/* Default values for the transport attributes */
> +	void *default_attr_values;

Should this really be a void *?

> +extern struct scsi_transport_template blank_transport_template;

Should be in scsi_priv.h as it's not exported, no?