Re: [PATCH] ses: Handle non-unique element descriptors

James Bottomley
2011-07-04T15:35:28Z

On Mon, 2011-07-04 at 20:46 +0530, Ankit Jain wrote:
> Some SES devices give non-unique Element Descriptors as part of the
> Element Descriptor diag page. Since we use these for creating sysfs
> entries, they need to be unique.
>
> Eg:
> $ sg_ses -p 7 /dev/sg0
> FTS CORP TXS6_SAS20BPX12 0500
> enclosure services device
> Element descriptor In diagnostic page:
> generation code: 0x0
> element descriptor by type list
> Element type: Array device, subenclosure id: 0
> Overall descriptor: ArrayDevicesInSubEnclsr0
> Element 1 descriptor: ArrayDevice00
> Element 2 descriptor: ArrayDevice01
> Element 3 descriptor: ArrayDevice02
> Element 4 descriptor: ArrayDevice03
> Element 5 descriptor: ArrayDevice03
> Element 6 descriptor: ArrayDevice03
> Element 7 descriptor: ArrayDevice03
> Element 8 descriptor: ArrayDevice03
> Element 9 descriptor: ArrayDevice03
> Element 10 descriptor: ArrayDevice03
> Element 11 descriptor: ArrayDevice03
> Element 12 descriptor: ArrayDevice03

What is the external visible labelling of this topology? It's
completely weird that the enclosure would burn in non-unique names
unless there's some reason for it.

> Signed-off-by: Ankit Jain <jankit@suse.de>
>
> ---
> diff --git a/drivers/misc/enclosure.c b/drivers/misc/enclosure.c
> index 00e5fca..8087055 100644
> --- a/drivers/misc/enclosure.c
> +++ b/drivers/misc/enclosure.c
> @@ -239,6 +239,24 @@ static void enclosure_component_release(struct device *dev)
> put_device(dev->parent);
> }
>
> +struct enclosure_component *
> +enclosure_component_find_by_name(struct enclosure_device *edev,
> + const char *name)
> +{
> + int i;
> + if (!edev || !name || !name[0])
> + return NULL;
> +
> + for (i=0; i<edev->components; i++) {
> + struct enclosure_component *ecomp = &edev->component[i];
> + if (ecomp->number != -1 && !strcmp(dev_name(&ecomp->cdev), name))
> + return ecomp;
> + }
> +
> + return NULL;
> +}
> +EXPORT_SYMBOL_GPL(enclosure_component_find_by_name);
> +
> static const struct attribute_group *enclosure_groups[];
>
> /**
> diff --git a/drivers/scsi/ses.c b/drivers/scsi/ses.c
> index eba183c..abfd962 100644
> --- a/drivers/scsi/ses.c
> +++ b/drivers/scsi/ses.c
> @@ -421,12 +421,17 @@ static void ses_enclosure_data_process(struct enclosure_device *edev,
> if (type_ptr[0] == ENCLOSURE_COMPONENT_DEVICE ||
> type_ptr[0] == ENCLOSURE_COMPONENT_ARRAY_DEVICE) {
>
> - if (create)
> + if (create) {
> + if (enclosure_component_find_by_name(edev, name))
> + /* name is not unique, already used
> + * set to NULL, so that enclosure_component_register
> + * will assign us a new one */
> + name = NULL;

This just assigns a random name ... if we actually have one, we should
probably just make it unique.

James

> ecomp = enclosure_component_register(edev,
> components++,
> type_ptr[0],
> name);
> - else
> + } else
> ecomp = &edev->component[components++];
>
> if (!IS_ERR(ecomp) && addl_desc_ptr)
> diff --git a/include/linux/enclosure.h b/include/linux/enclosure.h
> index 9a33c5f..7e48ba9 100644
> --- a/include/linux/enclosure.h
> +++ b/include/linux/enclosure.h
> @@ -122,6 +122,8 @@ void enclosure_unregister(struct enclosure_device *);
> struct enclosure_component *
> enclosure_component_register(struct enclosure_device *, unsigned int,
> enum enclosure_component_type, const char *);
> +struct enclosure_component *
> +enclosure_component_find_by_name(struct enclosure_device *, const char *);
> int enclosure_add_device(struct enclosure_device *enclosure, int component,
> struct device *dev);
> int enclosure_remove_device(struct enclosure_device *, struct device *);