[PATCH 3/3] kernel-device: simplify handling of platform/pci/pnp/sdio parent

[Dan Williams]

On Thu, 2017-02-23 at 14:11 -0800, Ben Chan wrote:
> On Thu, Feb 23, 2017 at 2:07 PM, Dan Williams <dcbw at redhat.com>
> wrote:
> 
> > On Thu, 2017-02-23 at 11:45 -0800, Ben Chan wrote:
> > > This patch simplifies the handling of platform/pci/pnp/sdio
> > > device in
> > > find_physical_gudevdevice(). When the code finds the first parent
> > > device
> > > under the subsystem 'platform', 'pci', 'pnp', or 'sdio', it stops
> > > traversing up the device tree, so there is no need to keep track
> > > of
> > > whether a platform / pci / pnp / sdio device has previously been
> > > visited
> > > via those is_platform/is_pci/is_pnp/is_sdio Boolean variables.
> > 
> > I'm not sure this will work in all cases, though I should verify
> > this.
> > Depending on the actual bus type, the common parent for all ports
> > exposed by that device might be a couple parents up the chain.
> > 
> > 
> 
> That's also what I was wondering when reading through the original
> code. If
> that's the case, the original code would have stopped too soon when
> traversing up the chain.

I tested the 3 cards that I think are relevant here (two PCI that don't
use an internal USB hub and one PCMCIA) and I think we're fine.  So
this patch is OK with me.

For all these devices (Option Nozomi, Sony Ericsson GC89, Sierra 860)
the unique parent is the immediate parent of the tty in question.

Dan

> 
> > I do still have PCMCIA (Sierra 860) and PCI (Option Nozomi) WWAN
> > cards
> > that I can check to see if this is the case, though I'll have to
> > install an older OS that still supports PCMCIA.
> > 
> > The "platform" case IIRC is when a serial device is attached to the
> > onboard serial port, so perhaps that one is OK since there will
> > only
> > ever be one device on those ports.
> > 
> > Dan
> > 
> > > ---
> > >  src/kerneldevice/mm-kernel-device-udev.c | 23 ++++++----------
> > > ----
> > > ---
> > >  1 file changed, 6 insertions(+), 17 deletions(-)
> > > 
> > > diff --git a/src/kerneldevice/mm-kernel-device-udev.c
> > > b/src/kerneldevice/mm-kernel-device-udev.c
> > > index 763ccf86..ab75aa34 100644
> > > --- a/src/kerneldevice/mm-kernel-device-udev.c
> > > +++ b/src/kerneldevice/mm-kernel-device-udev.c
> > > @@ -177,8 +177,7 @@ find_physical_gudevdevice (GUdevDevice
> > > *child)
> > >      GUdevDevice *physdev = NULL;
> > >      const char *subsys, *type, *name;
> > >      guint32 i = 0;
> > > -    gboolean is_usb = FALSE, is_pci = FALSE, is_pcmcia = FALSE,
> > > is_platform = FALSE;
> > > -    gboolean is_pnp = FALSE, is_sdio = FALSE;
> > > +    gboolean is_usb = FALSE, is_pcmcia = FALSE;
> > > 
> > >      g_return_val_if_fail (child != NULL, NULL);
> > > 
> > > @@ -220,21 +219,11 @@ find_physical_gudevdevice (GUdevDevice
> > > *child)
> > >                      if (physdev)
> > >                          break;
> > >                  }
> > > -            } else if (is_platform || !strcmp (subsys,
> > > "platform"))
> > > {
> > > -                /* Take the first platform parent as the
> > > physical
> > > device */
> > > -                is_platform = TRUE;
> > > -                physdev = iter;
> > > -                break;
> > > -            } else if (is_pci || !strcmp (subsys, "pci")) {
> > > -                is_pci = TRUE;
> > > -                physdev = iter;
> > > -                break;
> > > -            } else if (is_pnp || !strcmp (subsys, "pnp")) {
> > > -                is_pnp = TRUE;
> > > -                physdev = iter;
> > > -                break;
> > > -            } else if (is_sdio || !strcmp (subsys, "sdio")) {
> > > -                is_sdio = TRUE;
> > > +            } else if (!strcmp (subsys, "platform") ||
> > > +                       !strcmp (subsys, "pci") ||
> > > +                       !strcmp (subsys, "pnp") ||
> > > +                       !strcmp (subsys, "sdio")) {
> > > +                /* Take the first parent as the physical device
> > > */
> > >                  physdev = iter;
> > >                  break;
> > >              }