[PATCH RESEND] [SCSI] aic94xx: Remove broken fallback for missing 'Ctrl-A' user settings

[PATCH RESEND] [SCSI] aic94xx: Remove broken fallback for missing 'Ctrl-A' user settings

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 1985 bytes --]

asd_process_ctrl_a_user() attempts to find user settings in flash, and
if they are missing it prepares a substitute structure containing
default values for PHY settings.  But having done so, it will still
try to read user settings - from some random address in flash, as the
local variable 'offs' has not been initialised.

Since asd_common_setup() already sets default PHY settings, there
seems to be no need to repeat them here, and we can just return 0.

Compile-tested only.

Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
---
 drivers/scsi/aic94xx/aic94xx_sds.c | 18 ++----------------
 1 file changed, 2 insertions(+), 16 deletions(-)

diff --git a/drivers/scsi/aic94xx/aic94xx_sds.c b/drivers/scsi/aic94xx/aic94xx_sds.c
index edb43fd..f5d51d2 100644
--- a/drivers/scsi/aic94xx/aic94xx_sds.c
+++ b/drivers/scsi/aic94xx/aic94xx_sds.c
@@ -981,29 +981,15 @@ static int asd_process_ctrla_phy_settings(struct asd_ha_struct *asd_ha,
 static int asd_process_ctrl_a_user(struct asd_ha_struct *asd_ha,
 				   struct asd_flash_dir *flash_dir)
 {
-	int err, i;
+	int err;
 	u32 offs, size;
 	struct asd_ll_el *el;
 	struct asd_ctrla_phy_settings *ps;
-	struct asd_ctrla_phy_settings dflt_ps;
 
 	err = asd_find_flash_de(flash_dir, FLASH_DE_CTRL_A_USER, &offs, &size);
 	if (err) {
 		ASD_DPRINTK("couldn't find CTRL-A user settings section\n");
-		ASD_DPRINTK("Creating default CTRL-A user settings section\n");
-
-		dflt_ps.id0 = 'h';
-		dflt_ps.num_phys = 8;
-		for (i =0; i < ASD_MAX_PHYS; i++) {
-			memcpy(dflt_ps.phy_ent[i].sas_addr,
-			       asd_ha->hw_prof.sas_addr, SAS_ADDR_SIZE);
-			dflt_ps.phy_ent[i].sas_link_rates = 0x98;
-			dflt_ps.phy_ent[i].flags = 0x0;
-			dflt_ps.phy_ent[i].sata_link_rates = 0x0;
-		}
-
-		size = sizeof(struct asd_ctrla_phy_settings);
-		ps = &dflt_ps;
+		return 0;
 	}
 
 	if (size == 0)


-- 
Ben Hutchings
Never attribute to conspiracy what can adequately be explained by stupidity.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH RESEND] [SCSI] aic94xx: Remove broken fallback for missing 'Ctrl-A' user settings

[Christoph Hellwig]

On Sun, Jun 08, 2014 at 11:37:44PM +0100, Ben Hutchings wrote:
> asd_process_ctrl_a_user() attempts to find user settings in flash, and
> if they are missing it prepares a substitute structure containing
> default values for PHY settings.  But having done so, it will still
> try to read user settings - from some random address in flash, as the
> local variable 'offs' has not been initialised.
> 
> Since asd_common_setup() already sets default PHY settings, there
> seems to be no need to repeat them here, and we can just return 0.
> 
> Compile-tested only.
> 
> Signed-off-by: Ben Hutchings <ben@decadent.org.uk>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Can I get another review, too please?

Re: [PATCH RESEND] [SCSI] aic94xx: Remove broken fallback for missing 'Ctrl-A' user settings

[Martin K. Petersen]

>>>>> "Christoph" == Christoph Hellwig <hch@infradead.org> writes:

Christoph> On Sun, Jun 08, 2014 at 11:37:44PM +0100, Ben Hutchings wrote:
>> asd_process_ctrl_a_user() attempts to find user settings in flash,
>> and if they are missing it prepares a substitute structure containing
>> default values for PHY settings.  But having done so, it will still
>> try to read user settings - from some random address in flash, as the
>> local variable 'offs' has not been initialised.
>> 
>> Since asd_common_setup() already sets default PHY settings, there
>> seems to be no need to repeat them here, and we can just return 0.
>> 
>> Compile-tested only.

If I read this correctly we'll get the link rates initialized but the
sas_addr will no longer be populated for each phy_desc.

The old code copied the board global sas_addr into dflt_ps before
calling asd_process_ctrla_phy_settings(). Whereas asd_common_setup()
does not populate the sas_addr.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH RESEND] [SCSI] aic94xx: Remove broken fallback for missing 'Ctrl-A' user settings

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 1290 bytes --]

On Wed, 2014-06-25 at 22:44 -0400, Martin K. Petersen wrote:
> >>>>> "Christoph" == Christoph Hellwig <hch@infradead.org> writes:
> 
> Christoph> On Sun, Jun 08, 2014 at 11:37:44PM +0100, Ben Hutchings wrote:
> >> asd_process_ctrl_a_user() attempts to find user settings in flash,
> >> and if they are missing it prepares a substitute structure containing
> >> default values for PHY settings.  But having done so, it will still
> >> try to read user settings - from some random address in flash, as the
> >> local variable 'offs' has not been initialised.
> >> 
> >> Since asd_common_setup() already sets default PHY settings, there
> >> seems to be no need to repeat them here, and we can just return 0.
> >> 
> >> Compile-tested only.
> 
> If I read this correctly we'll get the link rates initialized but the
> sas_addr will no longer be populated for each phy_desc.
> 
> The old code copied the board global sas_addr into dflt_ps before
> calling asd_process_ctrla_phy_settings(). Whereas asd_common_setup()
> does not populate the sas_addr.

The old code also reassigned ps before calling
asd_process_ctrla_phy_settings(), so dflt_ps was not actually used.

Ben.

-- 
Ben Hutchings
Quantity is no substitute for quality, but it's the only one we've got.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 811 bytes --]