[PATCH] SCSI/qla2xxx: handle sysfs errors

[PATCH] SCSI/qla2xxx: handle sysfs errors

[Jeff Garzik]

Signed-off-by: Jeff Garzik <jeff@garzik.org>

---

 drivers/scsi/qla2xxx/qla_attr.c |   52 +++++++++++++++++++++++++++++++---------
 drivers/scsi/qla2xxx/qla_gbl.h  |    4 ---
 drivers/scsi/qla2xxx/qla_os.c   |    7 ++++-

diff --git a/drivers/scsi/qla2xxx/qla_attr.c b/drivers/scsi/qla2xxx/qla_attr.c
index ee75a71..54e4a60 100644
--- a/drivers/scsi/qla2xxx/qla_attr.c
+++ b/drivers/scsi/qla2xxx/qla_attr.c
@@ -379,22 +379,52 @@ static struct bin_attribute sysfs_sfp_at
 	.read = qla2x00_sysfs_read_sfp,
 };
 
-void
+int
 qla2x00_alloc_sysfs_attr(scsi_qla_host_t *ha)
 {
 	struct Scsi_Host *host = ha->host;
-
-	sysfs_create_bin_file(&host->shost_gendev.kobj, &sysfs_fw_dump_attr);
-	sysfs_create_bin_file(&host->shost_gendev.kobj, &sysfs_nvram_attr);
-	sysfs_create_bin_file(&host->shost_gendev.kobj, &sysfs_optrom_attr);
-	sysfs_create_bin_file(&host->shost_gendev.kobj,
-	    &sysfs_optrom_ctl_attr);
+	int rc;
+
+	rc = sysfs_create_bin_file(&host->shost_gendev.kobj,
+				   &sysfs_fw_dump_attr);
+	if (rc) goto err;
+	rc = sysfs_create_bin_file(&host->shost_gendev.kobj,
+				   &sysfs_nvram_attr);
+	if (rc) goto err_fwd;
+	rc = sysfs_create_bin_file(&host->shost_gendev.kobj,
+				   &sysfs_optrom_attr);
+	if (rc) goto err_nvr;
+	rc = sysfs_create_bin_file(&host->shost_gendev.kobj,
+				   &sysfs_optrom_ctl_attr);
+	if (rc) goto err_optrom;
 	if (IS_QLA24XX(ha) || IS_QLA54XX(ha)) {
-		sysfs_create_bin_file(&host->shost_gendev.kobj,
-		    &sysfs_vpd_attr);
-		sysfs_create_bin_file(&host->shost_gendev.kobj,
-		    &sysfs_sfp_attr);
+		rc = sysfs_create_bin_file(&host->shost_gendev.kobj,
+					   &sysfs_vpd_attr);
+		if (rc) goto err_opt_ctl;
+		rc = sysfs_create_bin_file(&host->shost_gendev.kobj,
+					   &sysfs_sfp_attr);
+		if (rc) goto err_vpd;
 	}
+
+	return 0;
+
+err_vpd:
+	sysfs_remove_bin_file(&host->shost_gendev.kobj,
+				   &sysfs_vpd_attr);
+err_opt_ctl:
+	sysfs_remove_bin_file(&host->shost_gendev.kobj,
+				   &sysfs_optrom_ctl_attr);
+err_optrom:
+	sysfs_remove_bin_file(&host->shost_gendev.kobj,
+				   &sysfs_optrom_attr);
+err_nvr:
+	sysfs_remove_bin_file(&host->shost_gendev.kobj,
+				   &sysfs_nvram_attr);
+err_fwd:
+	sysfs_remove_bin_file(&host->shost_gendev.kobj,
+				   &sysfs_fw_dump_attr);
+err:
+	return rc;
 }
 
 void
diff --git a/drivers/scsi/qla2xxx/qla_gbl.h b/drivers/scsi/qla2xxx/qla_gbl.h
index 7da6983..60ade19 100644
--- a/drivers/scsi/qla2xxx/qla_gbl.h
+++ b/drivers/scsi/qla2xxx/qla_gbl.h
@@ -296,9 +296,7 @@ struct class_device_attribute;
 extern struct class_device_attribute *qla2x00_host_attrs[];
 struct fc_function_template;
 extern struct fc_function_template qla2xxx_transport_functions;
-extern void qla2x00_alloc_sysfs_attr(scsi_qla_host_t *);
+extern int qla2x00_alloc_sysfs_attr(scsi_qla_host_t *);
 extern void qla2x00_free_sysfs_attr(scsi_qla_host_t *);
 extern void qla2x00_init_host_attr(scsi_qla_host_t *);
-extern void qla2x00_alloc_sysfs_attr(scsi_qla_host_t *);
-extern void qla2x00_free_sysfs_attr(scsi_qla_host_t *);
 #endif /* _QLA_GBL_H */
diff --git a/drivers/scsi/qla2xxx/qla_os.c b/drivers/scsi/qla2xxx/qla_os.c
index 3f20d76..d70162b 100644
--- a/drivers/scsi/qla2xxx/qla_os.c
+++ b/drivers/scsi/qla2xxx/qla_os.c
@@ -1638,7 +1638,9 @@ qla2x00_probe_one(struct pci_dev *pdev, 
 	if (ret)
 		goto probe_failed;
 
-	qla2x00_alloc_sysfs_attr(ha);
+	ret = qla2x00_alloc_sysfs_attr(ha);
+	if (ret)
+		goto probe_failed_rmhost;
 
 	qla2x00_init_host_attr(ha);
 
@@ -1658,6 +1660,9 @@ qla2x00_probe_one(struct pci_dev *pdev, 
 
 	return 0;
 
+probe_failed_rmhost:
+	scsi_remove_host(host);
+
 probe_failed:
 	qla2x00_free_device(ha);
 

Re: [PATCH] SCSI/qla2xxx: handle sysfs errors

[Andrew Vasquez]

On Wed, 11 Oct 2006, Jeff Garzik wrote:

> Signed-off-by: Jeff Garzik <jeff@garzik.org>
> 
> ---
> 
>  drivers/scsi/qla2xxx/qla_attr.c |   52 +++++++++++++++++++++++++++++++---------
>  drivers/scsi/qla2xxx/qla_gbl.h  |    4 ---
>  drivers/scsi/qla2xxx/qla_os.c   |    7 ++++-
> 
> diff --git a/drivers/scsi/qla2xxx/qla_attr.c b/drivers/scsi/qla2xxx/qla_attr.c
> index ee75a71..54e4a60 100644
> --- a/drivers/scsi/qla2xxx/qla_attr.c
> +++ b/drivers/scsi/qla2xxx/qla_attr.c
> @@ -379,22 +379,52 @@ static struct bin_attribute sysfs_sfp_at
>  	.read = qla2x00_sysfs_read_sfp,
>  };
>  
> -void
> +int
>  qla2x00_alloc_sysfs_attr(scsi_qla_host_t *ha)
>  {
>  	struct Scsi_Host *host = ha->host;
> -
> -	sysfs_create_bin_file(&host->shost_gendev.kobj, &sysfs_fw_dump_attr);
> -	sysfs_create_bin_file(&host->shost_gendev.kobj, &sysfs_nvram_attr);
> -	sysfs_create_bin_file(&host->shost_gendev.kobj, &sysfs_optrom_attr);
> -	sysfs_create_bin_file(&host->shost_gendev.kobj,
> -	    &sysfs_optrom_ctl_attr);
> +	int rc;
> +
> +	rc = sysfs_create_bin_file(&host->shost_gendev.kobj,
> +				   &sysfs_fw_dump_attr);
> +	if (rc) goto err;

NACK, please don't do this.  SYSFS entries, albiet important, aren't
necessarilly critical to a functioning driver.  I'd rather the driver
not error out.

Here's what I had stewing to address the must_check directives and
qla2xxx:

---

qla2xxx: Check return value of sysfs_create_bin_file() usage.

Signed-off-by: Andrew Vasquez <andrew.vasquez@qlogic.com>
---
 drivers/scsi/qla2xxx/qla_attr.c |   51 ++++++++++++++++++++++++--------------
 1 files changed, 32 insertions(+), 19 deletions(-)

diff --git a/drivers/scsi/qla2xxx/qla_attr.c b/drivers/scsi/qla2xxx/qla_attr.c
index ee75a71..285c8e8 100644
--- a/drivers/scsi/qla2xxx/qla_attr.c
+++ b/drivers/scsi/qla2xxx/qla_attr.c
@@ -379,21 +379,37 @@ static struct bin_attribute sysfs_sfp_at
 	.read = qla2x00_sysfs_read_sfp,
 };
 
+static struct sysfs_entry {
+	char *name;
+	struct bin_attribute *attr;
+	int is4GBp_only;
+} bin_file_entries[] = {
+	{ "fw_dump", &sysfs_fw_dump_attr, },
+	{ "nvram", &sysfs_nvram_attr, },
+	{ "optrom", &sysfs_optrom_attr, },
+	{ "optrom_ctl", &sysfs_optrom_ctl_attr, },
+	{ "vpd", &sysfs_vpd_attr, 1 },
+	{ "sfp", &sysfs_sfp_attr, 1 },
+	{ 0 },
+};
+
 void
 qla2x00_alloc_sysfs_attr(scsi_qla_host_t *ha)
 {
 	struct Scsi_Host *host = ha->host;
+	struct sysfs_entry *iter;
+	int ret;
 
-	sysfs_create_bin_file(&host->shost_gendev.kobj, &sysfs_fw_dump_attr);
-	sysfs_create_bin_file(&host->shost_gendev.kobj, &sysfs_nvram_attr);
-	sysfs_create_bin_file(&host->shost_gendev.kobj, &sysfs_optrom_attr);
-	sysfs_create_bin_file(&host->shost_gendev.kobj,
-	    &sysfs_optrom_ctl_attr);
-	if (IS_QLA24XX(ha) || IS_QLA54XX(ha)) {
-		sysfs_create_bin_file(&host->shost_gendev.kobj,
-		    &sysfs_vpd_attr);
-		sysfs_create_bin_file(&host->shost_gendev.kobj,
-		    &sysfs_sfp_attr);
+	for (iter = bin_file_entries; iter->name; iter++) {
+		if (iter->is4GBp_only && (!IS_QLA24XX(ha) && !IS_QLA54XX(ha)))
+			continue;
+
+		ret = sysfs_create_bin_file(&host->shost_gendev.kobj,
+		    iter->attr);
+		if (ret)
+			qla_printk(KERN_INFO, ha,
+			    "Unable to create sysfs %s binary attribute "
+			    "(%d).\n", iter->name, ret);
 	}
 }
 
@@ -401,17 +417,14 @@ void
 qla2x00_free_sysfs_attr(scsi_qla_host_t *ha)
 {
 	struct Scsi_Host *host = ha->host;
+	struct sysfs_entry *iter;
+
+	for (iter = bin_file_entries; iter->name; iter++) {
+		if (iter->is4GBp_only && (!IS_QLA24XX(ha) && !IS_QLA54XX(ha)))
+			continue;
 
-	sysfs_remove_bin_file(&host->shost_gendev.kobj, &sysfs_fw_dump_attr);
-	sysfs_remove_bin_file(&host->shost_gendev.kobj, &sysfs_nvram_attr);
-	sysfs_remove_bin_file(&host->shost_gendev.kobj, &sysfs_optrom_attr);
-	sysfs_remove_bin_file(&host->shost_gendev.kobj,
-	    &sysfs_optrom_ctl_attr);
-	if (IS_QLA24XX(ha) || IS_QLA54XX(ha)) {
-		sysfs_remove_bin_file(&host->shost_gendev.kobj,
-		    &sysfs_vpd_attr);
 		sysfs_remove_bin_file(&host->shost_gendev.kobj,
-		    &sysfs_sfp_attr);
+		    iter->attr);
 	}
 
 	if (ha->beacon_blink_led == 1)
-- 
1.4.3.rc2.g0503

Re: [PATCH] SCSI/qla2xxx: handle sysfs errors

[Jeff Garzik]

Andrew Vasquez wrote:
> NACK, please don't do this.  SYSFS entries, albiet important, aren't
> necessarilly critical to a functioning driver.  I'd rather the driver
> not error out.

As discussed before, the only errors thrown are either ENOMEM or EFAULT, 
both of which are quite serious.


> Here's what I had stewing to address the must_check directives and
> qla2xxx:

If you're gonna change it that much, might as well use attribute groups.

	Jeff

Re: [PATCH] SCSI/qla2xxx: handle sysfs errors

[Andrew Vasquez]

On Thu, 12 Oct 2006, Jeff Garzik wrote:

> Andrew Vasquez wrote:
> >NACK, please don't do this.  SYSFS entries, albiet important, aren't
> >necessarilly critical to a functioning driver.  I'd rather the driver
> >not error out.
> 
> As discussed before, the only errors thrown are either ENOMEM or EFAULT, 
> both of which are quite serious.

Absolutely.  But, given the relatively late-stage initialization of
these attributes (it's the last thing that gets done before probe()
completes)-- the driver has already allocated (successfully) memory,
intialized hardware and (possibly) presented storage, the complete
unwinding, seems over-kill.

> >Here's what I had stewing to address the must_check directives and
> >qla2xxx:
> 
> If you're gonna change it that much, might as well use attribute groups.

sysfs_create_group() and friends don't appear to support
binary attributes.