[PATCH v5 04/13] staging: unisys: Do not use 0 as the default bus root device number

[Don Zickus]

On Thu, Jun 04, 2015 at 05:19:44PM +0300, Dan Carpenter wrote:
> On Thu, Jun 04, 2015 at 09:22:40AM -0400, Benjamin Romer wrote:
> > diff --git a/drivers/staging/unisys/visorbus/visorchipset.c b/drivers/staging/unisys/visorbus/visorchipset.c
> > index 4dd0a07..618732b 100644
> > --- a/drivers/staging/unisys/visorbus/visorchipset.c
> > +++ b/drivers/staging/unisys/visorbus/visorchipset.c
> > @@ -56,6 +56,8 @@
> >  #define UNISYS_SPAR_ID_ECX 0x70537379
> >  #define UNISYS_SPAR_ID_EDX 0x34367261
> >  
> > +#define BUS_ROOT_DEVICE	UINT_MAX
> > +
> >  /*
> >   * Module parameters
> >   */
> > @@ -727,8 +729,8 @@ static int match_visorbus_dev_by_id(struct device *dev, void *data)
> >  	u32 bus_no = id->bus_no;
> >  	u32 dev_no = id->dev_no;
> >  
> > -	if (((bus_no == -1) || (vdev->chipset_bus_no == bus_no)) &&
> > -	    ((dev_no == -1) || (vdev->chipset_dev_no == dev_no)))
> > +	if ((vdev->chipset_bus_no == bus_no) &&
> > +	    (vdev->chipset_dev_no == dev_no))
> >  		return 1;
> 
> This talk about "root device" confuses me.  I thought that -1 was a wild
> card.  Anyway, we introduce the BUS_ROOT_DEVICE macro but don't use it.
> Shouldn't it be defined as 0 since that it the root device?

Hi Dan,

Thanks for the review.  My latest round of patch re-arrangement to make this
patchset bisectable makes this patch look awkward.  Awkward because as you
said BUS_ROOT_DEVIC isn't used.  However, it is used in patch 5 (the next
patch), but is also moved from a .c file to a .h to handle another corner
case in another .c file.  So patch5 is a better study of the usage and this
patch is the fallout of a patch written later and then re-arranged to be
earlier.

But lets back up to 'what is root device'.  Before unisys can load its io
drivers, it needs a virtual bus to sit on.  Their s-par sends 'create bus'
commands and I convert those into linux device creations.  When an io driver
is created later using a 'device create' command from the s-par,  it too
creates a linux device with its parent pointing to the 'bus root device' or
the initial virtual bus created.  This mimics what other virtual/real buses
are doing in linux (I believe).

So to back up a little bit, when the 'device create' command is sent from
the s-par, it provides a bus number to sit on and a device number to assign
it to.  Considering all the buses and devices are all lumped together under
one subsystem 'visorbus' in the device tree link-list, it is easy to walk
that link-list using the linux infrastructure.  The only problem is how to
separate an actual device from the actual bus it sits on when both have the
same bus no.

In that case, I use -1 to represent the 'bus root device'.  I agree
traditionally that infers a wild card.  I was scratching my head on a better
mechanism without re-implementing the wheel and at the same time get
something working.

Hopefully that clarifies the term of 'bus root device'.  Suggestions
welcomed. :-)



To answer your second question, why not use 0?  Originally I did, but unisys
explained to me that it is valid for their device numbering to start at 0.
Their testing uncovered this bug and hence the need for this patch.  Their
device numbering is dynamic but probably only goes as far as 8 or so.  I
chose -1 to be extremely safe. :-)

I can look to see if their docs have a MAX_DEVICE_NUMBER and use that
instead for the bus root device, if that would make more sense from a code
readability perspective and leave -1 as a possibility for a wild card in the
future.  Thoughts?

Cheers,
Don