[PATCH] ssb: return -ENOMEM on alloc fail (instead of CRC check's result)

[PATCH] ssb: return -ENOMEM on alloc fail (instead of CRC check's result)

[Rafał Miłecki]

Signed-off-by: Rafa? Mi?ecki <zajec5@gmail.com>
---
John: it's for 2.6.38.
---
 drivers/ssb/pci.c |    7 +++----
 1 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/ssb/pci.c b/drivers/ssb/pci.c
index 3226832..b5343ac 100644
--- a/drivers/ssb/pci.c
+++ b/drivers/ssb/pci.c
@@ -619,7 +619,7 @@ static int ssb_pci_sprom_get(struct ssb_bus *bus,
 			     struct ssb_sprom *sprom)
 {
 	const struct ssb_sprom *fallback;
-	int err = -ENOMEM;
+	int err;
 	u16 *buf;
 
 	if (!ssb_is_sprom_available(bus)) {
@@ -646,7 +646,7 @@ static int ssb_pci_sprom_get(struct ssb_bus *bus,
 
 	buf = kcalloc(SSB_SPROMSIZE_WORDS_R123, sizeof(u16), GFP_KERNEL);
 	if (!buf)
-		goto out;
+		return -ENOMEM;
 	bus->sprom_size = SSB_SPROMSIZE_WORDS_R123;
 	sprom_do_read(bus, buf);
 	err = sprom_check_crc(buf, bus->sprom_size);
@@ -656,7 +656,7 @@ static int ssb_pci_sprom_get(struct ssb_bus *bus,
 		buf = kcalloc(SSB_SPROMSIZE_WORDS_R4, sizeof(u16),
 			      GFP_KERNEL);
 		if (!buf)
-			goto out;
+			return -ENOMEM;
 		bus->sprom_size = SSB_SPROMSIZE_WORDS_R4;
 		sprom_do_read(bus, buf);
 		err = sprom_check_crc(buf, bus->sprom_size);
@@ -678,7 +678,6 @@ static int ssb_pci_sprom_get(struct ssb_bus *bus,
 
 out_free:
 	kfree(buf);
-out:
 	return err;
 }
 
-- 
1.6.0.4

[PATCH] ssb: fail registration for unknown SPROM revision

[Rafał Miłecki]

Signed-off-by: Rafa? Mi?ecki <zajec5@gmail.com>
---
As noticed my Michael we have this left from old times of limited amount of
revisions. It's not reliable to treat unknown as rev1.

Michael: would you like to add Reported-by?

John: it's for 2.6.38.
---
 drivers/ssb/pci.c |    8 +++-----
 1 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/ssb/pci.c b/drivers/ssb/pci.c
index b5343ac..41a7337 100644
--- a/drivers/ssb/pci.c
+++ b/drivers/ssb/pci.c
@@ -600,11 +600,9 @@ static int sprom_extract(struct ssb_bus *bus, struct ssb_sprom *out,
 		sprom_extract_r8(out, in);
 		break;
 	default:
-		ssb_printk(KERN_WARNING PFX "Unsupported SPROM"
-			   "  revision %d detected. Will extract"
-			   " v1\n", out->revision);
-		out->revision = 1;
-		sprom_extract_r123(out, in);
+		ssb_printk(KERN_ERR PFX "Unsupported SPROM revision %d"
+			   " detected\n", out->revision);
+		return -EINVAL;
 	}
 
 	if (out->boardflags_lo == 0xFFFF)
-- 
1.6.0.4

[PATCH] ssb: return -ENOMEM on alloc fail (instead of CRC check's result)

[Rafał Miłecki]

W dniu 3 listopada 2010 23:28 u?ytkownik Rafa? Mi?ecki
<zajec5@gmail.com> napisa?:
> 1.6.0.4

Whoops, that old git doesn't nicely numerate patches like 1/2, 2/2. Sorry.

-- 
Rafa?

[PATCH] ssb: fail registration for unknown SPROM revision

[Michael Büsch]

On Wed, 2010-11-03 at 23:28 +0100, Rafa? Mi?ecki wrote: 
> Signed-off-by: Rafa? Mi?ecki <zajec5@gmail.com>
> ---
> As noticed my Michael we have this left from old times of limited amount of
> revisions. It's not reliable to treat unknown as rev1.
> 
> Michael: would you like to add Reported-by?

Isn't that only for bugs? This isn't a bug.
But you can add my acked-by, if that's desired.

> John: it's for 2.6.38.
> ---
>  drivers/ssb/pci.c |    8 +++-----
>  1 files changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/ssb/pci.c b/drivers/ssb/pci.c
> index b5343ac..41a7337 100644
> --- a/drivers/ssb/pci.c
> +++ b/drivers/ssb/pci.c
> @@ -600,11 +600,9 @@ static int sprom_extract(struct ssb_bus *bus, struct ssb_sprom *out,
>  		sprom_extract_r8(out, in);
>  		break;
>  	default:
> -		ssb_printk(KERN_WARNING PFX "Unsupported SPROM"
> -			   "  revision %d detected. Will extract"
> -			   " v1\n", out->revision);
> -		out->revision = 1;
> -		sprom_extract_r123(out, in);
> +		ssb_printk(KERN_ERR PFX "Unsupported SPROM revision %d"
> +			   " detected\n", out->revision);
> +		return -EINVAL;
>  	}
>  
>  	if (out->boardflags_lo == 0xFFFF)


-- 
Greetings Michael.

[PATCH] ssb: fail registration for unknown SPROM revision

[John W. Linville]

On Wed, Nov 03, 2010 at 11:28:46PM +0100, Rafa? Mi?ecki wrote:
> Signed-off-by: Rafa? Mi?ecki <zajec5@gmail.com>
> ---
> As noticed my Michael we have this left from old times of limited amount of
> revisions. It's not reliable to treat unknown as rev1.
> 
> Michael: would you like to add Reported-by?
> 
> John: it's for 2.6.38.
> ---
>  drivers/ssb/pci.c |    8 +++-----
>  1 files changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/ssb/pci.c b/drivers/ssb/pci.c
> index b5343ac..41a7337 100644
> --- a/drivers/ssb/pci.c
> +++ b/drivers/ssb/pci.c
> @@ -600,11 +600,9 @@ static int sprom_extract(struct ssb_bus *bus, struct ssb_sprom *out,
>  		sprom_extract_r8(out, in);
>  		break;
>  	default:
> -		ssb_printk(KERN_WARNING PFX "Unsupported SPROM"
> -			   "  revision %d detected. Will extract"
> -			   " v1\n", out->revision);
> -		out->revision = 1;
> -		sprom_extract_r123(out, in);
> +		ssb_printk(KERN_ERR PFX "Unsupported SPROM revision %d"
> +			   " detected\n", out->revision);
> +		return -EINVAL;
>  	}
>  
>  	if (out->boardflags_lo == 0xFFFF)

I think this is going to make my b43 PCI-E card not work...I'll try
it and get back to you...

John
-- 
John W. Linville		Someday the world will need a hero, and you
linville at tuxdriver.com			might be all we have.  Be ready.

[PATCH] ssb: fail registration for unknown SPROM revision

[Michael Büsch]

On Tue, 2010-11-16 at 16:23 -0500, John W. Linville wrote: 
> > -		ssb_printk(KERN_WARNING PFX "Unsupported SPROM"
> > -			   "  revision %d detected. Will extract"
> > -			   " v1\n", out->revision);
> > -		out->revision = 1;
> > -		sprom_extract_r123(out, in);
> > +		ssb_printk(KERN_ERR PFX "Unsupported SPROM revision %d"
> > +			   " detected\n", out->revision);
> > +		return -EINVAL;
> >  	}
> >  
> >  	if (out->boardflags_lo == 0xFFFF)
> 
> I think this is going to make my b43 PCI-E card not work...I'll try
> it and get back to you...

Hm, what version does it report?

-- 
Greetings Michael.

[PATCH] ssb: fail registration for unknown SPROM revision

[Rafał Miłecki]

2010/11/18 John W. Linville <linville@tuxdriver.com>:
> On Tue, Nov 16, 2010 at 04:23:21PM -0500, John W. Linville wrote:
>> On Wed, Nov 03, 2010 at 11:28:46PM +0100, Rafa? Mi?ecki wrote:
>> > Signed-off-by: Rafa? Mi?ecki <zajec5@gmail.com>
>> > ---
>> > As noticed my Michael we have this left from old times of limited amount of
>> > revisions. It's not reliable to treat unknown as rev1.
>> >
>> > Michael: would you like to add Reported-by?
>> >
>> > John: it's for 2.6.38.
>> > ---
>> > ?drivers/ssb/pci.c | ? ?8 +++-----
>> > ?1 files changed, 3 insertions(+), 5 deletions(-)
>> >
>> > diff --git a/drivers/ssb/pci.c b/drivers/ssb/pci.c
>> > index b5343ac..41a7337 100644
>> > --- a/drivers/ssb/pci.c
>> > +++ b/drivers/ssb/pci.c
>> > @@ -600,11 +600,9 @@ static int sprom_extract(struct ssb_bus *bus, struct ssb_sprom *out,
>> > ? ? ? ? ? ? sprom_extract_r8(out, in);
>> > ? ? ? ? ? ? break;
>> > ? ? default:
>> > - ? ? ? ? ? ssb_printk(KERN_WARNING PFX "Unsupported SPROM"
>> > - ? ? ? ? ? ? ? ? ? ? ?" ?revision %d detected. Will extract"
>> > - ? ? ? ? ? ? ? ? ? ? ?" v1\n", out->revision);
>> > - ? ? ? ? ? out->revision = 1;
>> > - ? ? ? ? ? sprom_extract_r123(out, in);
>> > + ? ? ? ? ? ssb_printk(KERN_ERR PFX "Unsupported SPROM revision %d"
>> > + ? ? ? ? ? ? ? ? ? ? ?" detected\n", out->revision);
>> > + ? ? ? ? ? return -EINVAL;
>> > ? ? }
>> >
>> > ? ? if (out->boardflags_lo == 0xFFFF)
>>
>> I think this is going to make my b43 PCI-E card not work...I'll try
>> it and get back to you...
>
> Yeah...
>
> [ ?125.520348] b43-pci-bridge 0000:04:00.0: enabling device (0000 -> 0002)
> [ ?125.520359] b43-pci-bridge 0000:04:00.0: PCI INT A -> GSI 18 (level, low) -> IRQ 18
> [ ?125.520373] b43-pci-bridge 0000:04:00.0: setting latency timer to 64
> [ ?125.529241] ssb: Core 0 found: ChipCommon (cc 0x800, rev 0x11, vendor 0x4243)
> [ ?125.529255] ssb: Core 1 found: IEEE 802.11 (cc 0x812, rev 0x0A, vendor 0x4243)
> [ ?125.529268] ssb: Core 2 found: USB 1.1 Host (cc 0x817, rev 0x03, vendor 0x4243)
> [ ?125.529280] ssb: Core 3 found: PCI-E (cc 0x820, rev 0x01, vendor 0x4243)
> [ ?125.545750] ssb: WARNING: Invalid SPROM CRC (corrupt SPROM)
> [ ?125.545752] ssb: Unsupported SPROM revision 255 detected
> [ ?125.545797] ssb: Failed to register PCI version of SSB with error -22
> [ ?125.545810] b43-pci-bridge 0000:04:00.0: PCI INT A disabled
> [ ?125.545827] b43-pci-bridge: probe of 0000:04:00.0 failed with error -22

Hm, now I wonder if we should create sth like list of devices with
invalid SPROM, or maybe we should keep current fallback method?

-- 
Rafa?

[PATCH] ssb: fail registration for unknown SPROM revision

[Michael Büsch]

On Thu, 2010-11-18 at 11:27 -0500, John W. Linville wrote: 
> On Wed, Nov 17, 2010 at 06:12:56PM +0100, Michael B?sch wrote:
> > On Tue, 2010-11-16 at 16:23 -0500, John W. Linville wrote: 
> > > > -		ssb_printk(KERN_WARNING PFX "Unsupported SPROM"
> > > > -			   "  revision %d detected. Will extract"
> > > > -			   " v1\n", out->revision);
> > > > -		out->revision = 1;
> > > > -		sprom_extract_r123(out, in);
> > > > +		ssb_printk(KERN_ERR PFX "Unsupported SPROM revision %d"
> > > > +			   " detected\n", out->revision);
> > > > +		return -EINVAL;
> > > >  	}
> > > >  
> > > >  	if (out->boardflags_lo == 0xFFFF)
> > > 
> > > I think this is going to make my b43 PCI-E card not work...I'll try
> > > it and get back to you...
> > 
> > Hm, what version does it report?

> [ 1036.293865] ssb: Unsupported SPROM  revision 255 detected. Will extract v1

So what about specialcasing 255 instead of defaulting to 1 in general?

if (rev == 255)
rev = 1;

255 basically means "Vendor forgot to set this field". So it would only
default to 1 for those broken sproms.

-- 
Greetings Michael.

[PATCH] ssb: fail registration for unknown SPROM revision

[Rafał Miłecki]

2010/11/18 Michael B?sch <mb@bu3sch.de>:
> On Thu, 2010-11-18 at 11:27 -0500, John W. Linville wrote:
>> On Wed, Nov 17, 2010 at 06:12:56PM +0100, Michael B?sch wrote:
>> > On Tue, 2010-11-16 at 16:23 -0500, John W. Linville wrote:
>> > > > - ? ? ? ? ? ? ? ssb_printk(KERN_WARNING PFX "Unsupported SPROM"
>> > > > - ? ? ? ? ? ? ? ? ? ? ? ? ?" ?revision %d detected. Will extract"
>> > > > - ? ? ? ? ? ? ? ? ? ? ? ? ?" v1\n", out->revision);
>> > > > - ? ? ? ? ? ? ? out->revision = 1;
>> > > > - ? ? ? ? ? ? ? sprom_extract_r123(out, in);
>> > > > + ? ? ? ? ? ? ? ssb_printk(KERN_ERR PFX "Unsupported SPROM revision %d"
>> > > > + ? ? ? ? ? ? ? ? ? ? ? ? ?" detected\n", out->revision);
>> > > > + ? ? ? ? ? ? ? return -EINVAL;
>> > > > ? ? ? ? }
>> > > >
>> > > > ? ? ? ? if (out->boardflags_lo == 0xFFFF)
>> > >
>> > > I think this is going to make my b43 PCI-E card not work...I'll try
>> > > it and get back to you...
>> >
>> > Hm, what version does it report?
>
>> [ 1036.293865] ssb: Unsupported SPROM ?revision 255 detected. Will extract v1
>
> So what about specialcasing 255 instead of defaulting to 1 in general?
>
> if (rev == 255)
> rev = 1;
>
> 255 basically means "Vendor forgot to set this field". So it would only
> default to 1 for those broken sproms.

Will work as long as there won't appear new vendor who will forget to
set this and will use new SPROM...

But hopefully it won't happen and it should not hurt too much to
register device with incorrectly parsed SPROM.

-- 
Rafa?

[PATCH] ssb: fail registration for unknown SPROM revision

[Michael Büsch]

On Thu, 2010-11-18 at 17:44 +0100, Rafa? Mi?ecki wrote: 
> 2010/11/18 Michael B?sch <mb@bu3sch.de>:
> >> [ 1036.293865] ssb: Unsupported SPROM  revision 255 detected. Will extract v1
> >
> > So what about specialcasing 255 instead of defaulting to 1 in general?
> >
> > if (rev == 255)
> > rev = 1;
> >
> > 255 basically means "Vendor forgot to set this field". So it would only
> > default to 1 for those broken sproms.
> 
> Will work as long as there won't appear new vendor who will forget to
> set this and will use new SPROM...

The old code will break for that, too.

> But hopefully it won't happen and it should not hurt too much to
> register device with incorrectly parsed SPROM.

If it would really succeed to initialize the device, this would be a
regulatory issue, because the sprom contains various power amplifier
calibration data. I think it should rather fail and be fixed correctly
instead of incorrectly using rev1 in that case.

-- 
Greetings Michael.

[PATCH] ssb: fail registration for unknown SPROM revision

[Larry Finger]

On 11/18/2010 10:47 AM, Michael B?sch wrote:
> If it would really succeed to initialize the device, this would be a
> regulatory issue, because the sprom contains various power amplifier
> calibration data. I think it should rather fail and be fixed correctly
> instead of incorrectly using rev1 in that case.

I agree that it is better to fail than use incorrect power data.

Would it be useful if the SPROM data were logged when the revision is crap?

John: could you dump and post the data from yours? I would like to see how bad
it is.

Larry

[PATCH] ssb: fail registration for unknown SPROM revision

[Michael Büsch]

On Thu, 2010-11-18 at 11:02 -0600, Larry Finger wrote: 
> On 11/18/2010 10:47 AM, Michael B?sch wrote:
> > If it would really succeed to initialize the device, this would be a
> > regulatory issue, because the sprom contains various power amplifier
> > calibration data. I think it should rather fail and be fixed correctly
> > instead of incorrectly using rev1 in that case.
> 
> I agree that it is better to fail than use incorrect power data.
> 
> Would it be useful if the SPROM data were logged when the revision is crap?


We need to keep in mind that there will be no new SSB devices.
It seems pretty much EOL'ed by Broadcom.
So I'm not sure whether this would be of any use or just random dead
code.

-- 
Greetings Michael.

[PATCH] ssb: fail registration for unknown SPROM revision

[Larry Finger]

On 11/18/2010 11:07 AM, Michael B?sch wrote:
> On Thu, 2010-11-18 at 11:02 -0600, Larry Finger wrote: 
>> On 11/18/2010 10:47 AM, Michael B?sch wrote:
>>> If it would really succeed to initialize the device, this would be a
>>> regulatory issue, because the sprom contains various power amplifier
>>> calibration data. I think it should rather fail and be fixed correctly
>>> instead of incorrectly using rev1 in that case.
>>
>> I agree that it is better to fail than use incorrect power data.
>>
>> Would it be useful if the SPROM data were logged when the revision is crap?
> 
> 
> We need to keep in mind that there will be no new SSB devices.
> It seems pretty much EOL'ed by Broadcom.
> So I'm not sure whether this would be of any use or just random dead
> code.

Good point. When we get the data for the one case we know exists, we will have a
better idea if a special quirk for this case is feasible. Assuming that this is
not the only example of this hardware, then we might limit the breakage from
this patch. At least the random code would be needed and useful in keeping our
maintainer happy, which has a some merit.

Larry