ses: Handle non-unique element descriptors

PATCH Superseded Ankit Jain 2 participants 5 messages Jul 4, 2011 – Jul 11, 2011

Subsystems (2)

SCSI Char and misc drivers

lore

Share & tools

lore

Ankit Jain

author

2011-07-04 15:14 UTC #0 lore raw

PATCH 1/1 ses: Handle non-unique element descriptors

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

Based on scsi-misc

Signed-off-by Ankit Jain <[email protected]>

---

drivers/misc/enclosure.c +18 -0

index 00e5fca..8087055 100644
@@ -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[];
 
 /**

drivers/scsi/ses.c +7 -2

index eba183c..abfd962 100644
@@ -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;
 					ecomp =	enclosure_component_register(edev,
 									     components++,
 									     type_ptr[0],
 									     name);
-				else
+				} else
 					ecomp = &edev->component[components++];
 
 				if (!IS_ERR(ecomp) && addl_desc_ptr)

include/linux/enclosure.h +2 -0

index 9a33c5f..7e48ba9 100644
@@ -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 *);

Signature · 2 lines

Ankit Jain
SUSE Labs

James Bottomley

maintainer

2011-07-04 15:35 UTC #1 lore raw

Quoted 17 lines · Ankit Jain

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

Quoted 8 lines

      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.

Quoted 37 lines

Signed-off-by Ankit Jain <[email protected]>

---

drivers/misc/enclosure.c +18 -0

index 00e5fca..8087055 100644
@@ -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[];
 
 /**

drivers/scsi/ses.c +0 -0

index eba183c..abfd962 100644

drivers/scsi/ses.c static void ses_enclosure_data_process(struct enclosure_device *edev, · line 429

Quoted 10 lines

drivers/scsi/ses.c +6 -1

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

Quoted 22 lines

drivers/scsi/ses.c +1 -1

 					ecomp =	enclosure_component_register(edev,
 									     components++,
 									     type_ptr[0],
 									     name);
-				else
+				} else
 					ecomp = &edev->component[components++];
 
 				if (!IS_ERR(ecomp) && addl_desc_ptr)

include/linux/enclosure.h +2 -0

index 9a33c5f..7e48ba9 100644
@@ -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 *);

Ankit Jain

author

2011-07-04 22:57 UTC #2 lore raw

Quoted 27 lines · James Bottomley

Quoted 17 lines · Ankit Jain

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

Quoted 8 lines

      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

Quoted 3 lines

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.

I'm not sure what you mean by "external visible labelling". The system has a SAS expanded backplane. I don't have access to the hardware now, but p7 looked like this:

$ 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
    Element type: SAS connector, subenclosure id: 0
      Overall descriptor: ConnectorsInSubEnclsr0
      Element 1 descriptor: Connector04
      Element 2 descriptor: Connector05
      Element 3 descriptor: Connector06
      Element 4 descriptor: Connector07
      Element 5 descriptor: Connector00
      Element 6 descriptor: Connector01
      Element 7 descriptor: Connector02
      Element 8 descriptor: Connector03
      Element 9 descriptor: Connector08
      Element 10 descriptor: Connector09
      Element 11 descriptor: Connector10
      Element 12 descriptor: Connector11
    Element type: Temperature sense, subenclosure id: 0
      Overall descriptor: TempSensorsInSubEnclsr0
      Element 1 descriptor: TempSense01
      Element 2 descriptor: TempSense02
      Element 3 descriptor: TempSense03
    Element type: Voltage sensor, subenclosure id: 0
      Overall descriptor: VoltageSensorsInSubEnclsr0
      Element 1 descriptor: VoltageSense01
      Element 2 descriptor: VoltageSense02
      Element 3 descriptor: VoltageSense03
      Element 4 descriptor: VoltageSense04
    Element type: Current sensor, subenclosure id: 0
      Overall descriptor: CurrentSensorsInSubEnclsr0
      Element 1 descriptor: CurrentSense01
    Element type: Enclosure, subenclosure id: 0
      Overall descriptor: EnclosureElementInSubEnclsr0
      Element 1 descriptor: EnclosureElement01
    Element type: SAS expander, subenclosure id: 0
      Overall descriptor: SAS Expander
      Element 1 descriptor: Expander0

AFAICS, the spec doesn't seem to say that the names have to be unique. But, now that I think about it a bit, possibly some of the "array device slots" might have been vacant, and so descriptors could have been repeated. This could probably be checked with the status page.
But then I'm not sure, whether sysfs entries should or shouldn't be created for some cases (eg. status code!=OK).

Quoted 10 lines

Review
Quoted 7 lines
(quoted diff) +6 -1
-				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.

Agreed. Would something like (below) be acceptable? I'll post the patch separately, if it is:

---

drivers/misc/enclosure.c +24 -3

index 00e5fca..f4b53fd 100644
@@ -239,6 +239,23 @@ static void enclosure_component_release(struct device *dev)
 	put_device(dev->parent);
 }
 
+static 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;
+}
+
 static const struct attribute_group *enclosure_groups[];
 
 /**

@@ -276,9 +293,13 @@ enclosure_component_register(struct enclosure_device *edev,
 	ecomp->number = number;
 	cdev = &ecomp->cdev;
 	cdev->parent = get_device(&edev->edev);
-	if (name && name[0])
-		dev_set_name(cdev, "%s", name);
-	else
+
+	if (name && name[0]) {
+		if (enclosure_component_find_by_name (edev, name))
+			dev_set_name(cdev, "%s_%u", name, number);
+		else
+			dev_set_name(cdev, "%s", name);
+	} else
 		dev_set_name(cdev, "%u", number);
 
 	cdev->release = enclosure_component_release;

Signature · 2 lines

Ankit Jain
SUSE Labs

James Bottomley

maintainer

2011-07-04 23:00 UTC #3 lore raw

Quoted 28 lines · Ankit Jain

Quoted 27 lines · James Bottomley

Quoted 25 lines · Ankit Jain

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

Quoted 7 lines

Quoted 3 lines
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.
I'm not sure what you mean by "external visible labelling". The system has a SAS expanded backplane. I don't have access to the hardware now, but p7 looked like this:

The element descriptors usually correspond with labelling on the case of whatever it is the enclosure is embedded in. Their job is to describe the location of the device, so they usually get burned in with whatever the default labelling scheme for the physical device is. So what I want to know is what is the labelling scheme in this case?

James

Ankit Jain

author

2011-07-11 10:22 UTC #4 lore raw

Quoted 37 lines · James Bottomley

Quoted 28 lines · Ankit Jain
Quoted 27 lines · James Bottomley
Quoted 25 lines · Ankit Jain
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
Quoted 7 lines
Quoted 3 lines
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.
I'm not sure what you mean by "external visible labelling". The system has a SAS expanded backplane. I don't have access to the hardware now, but p7 looked like this:

Quoted 5 lines

The element descriptors usually correspond with labelling on the case of whatever it is the enclosure is embedded in. Their job is to describe the location of the device, so they usually get burned in with whatever the default labelling scheme for the physical device is. So what I want to know is what is the labelling scheme in this case?

I don't have direct access to the box, so I asked the person who does, and he says: "the labelling is just 1-12 unless I'm mistaken.".

The duplicate descriptors is a firmware bug, but since the specification doesn't say that these have to be unique, we should handle these, IMHO.

Signature · 2 lines

Ankit Jain
SUSE Labs