ATM bug found

ATM bug found

[Jeff Garzik]

Unlike 98% of the warnings of this type, this gcc warning does indeed 
seem to indicate a bug:

drivers/atm/zatm.c: In function ‘zatm_open’:
drivers/atm/zatm.c:919: warning: ‘pcr’ may be used uninitialized in this 
function

If alloc_shaper() argument 'unlimited' is true, then pcr is never 
assigned a value.  However, the caller of alloc_shaper() always tests 
the pcr value, regardless of whether or not 'unlimited' is true.

	Jeff

Re: ATM bug found

[Mitchell Blank Jr]

(trimmed cc:'s since, IMO, isn't really all that general interest)

Jeff Garzik wrote:
> drivers/atm/zatm.c: In function ?zatm_open?:
> drivers/atm/zatm.c:919: warning: ?pcr? may be used uninitialized in this 
> function

Yeah, looks like a bug.  Not very high-impact because:
  1. it only results in garbled data in vcc->qos.txtp which doesn't
     really mean much for UBR vcc's anyway
  2. zatm is a more-obscure-than-obscure piece of hardware.  I'm sure
     3c501.c has more users these days :-)

The fix is for alloc_shaper() should really do "*pcr = ATM_MAX_PCR" in
the "if (ubr)" stanza.  Chas, want to submit that in the next batch
of patches?

-Mitch

Re: ATM bug found

[chas williams - CONTRACTOR]

In message <20061001145829.GA83074@gaz.sfgoth.com>,Mitchell Blank Jr writes:
>The fix is for alloc_shaper() should really do "*pcr = ATM_MAX_PCR" in
>the "if (ubr)" stanza.  Chas, want to submit that in the next batch
>of patches?

i dont think you can do that.  pcr gets assigned to .min_pcr and .min_pcr
= ATM_MAX_PCR has a different meaning.  it should probably be set to 0.
there is no minimum or maximum pcr for ubr.  you get what you get.

Re: ATM bug found

[chas williams - CONTRACTOR]

In message <451FC182.6000502@garzik.org>,Jeff Garzik writes:
>If alloc_shaper() argument 'unlimited' is true, then pcr is never 
>assigned a value.  However, the caller of alloc_shaper() always tests 
>the pcr value, regardless of whether or not 'unlimited' is true.

when unlimited is true, this means ubr.  alloc_shaper() creates a queue
to use for all ubr (best effort) traffic.  ubr doesnt count against tx_bw
so its handled a bit differently.  alloc_shaper() should return a 0 for
the pcr since this gets assigned to the vcc's qos parameters.  min_pcr =
0 and max_pcr = 0 means "best effort".

still generates a warning from gcc though.

[ATM]: [zatm] always *pcr in alloc_shaper()

Signed-off-by: Chas Williams <chas@cmf.nrl.navy.mil>

diff --git a/drivers/atm/zatm.c b/drivers/atm/zatm.c
index c491ec4..083c5d3 100644
--- a/drivers/atm/zatm.c
+++ b/drivers/atm/zatm.c
@@ -800,6 +800,7 @@ static int alloc_shaper(struct atm_dev *
 		i = m = 1;
 		zatm_dev->ubr_ref_cnt++;
 		zatm_dev->ubr = shaper;
+		*pcr = 0;
 	}
 	else {
 		if (min) {

Re: ATM bug found

[Mitchell Blank Jr]

chas williams - CONTRACTOR wrote:
> still generates a warning from gcc though.

The warning is bogus in this case, though -- the only way for "*pcr" to
be unset is when alloc_shaper() returns non-zero

> +		*pcr = 0;

You're right, 0 is better than ATM_MAX_PCR here.

-Mitch