[review] aleksander/huawei-ndisdup-cdc-wdm-rebased-1.2

[Dan Williams]

On Sun, 2014-06-22 at 18:22 +0200, Aleksander Morgado wrote:
> On Thu, Jun 19, 2014 at 7:04 PM, Dan Williams <dcbw at redhat.com> wrote:
> >> > peek_port_at_for_data() seems kind ugly to me; can't we figure this out
> >> > when constructing the modem, instead of hitting up udev every time we
> >> > want to dial?  Plus during modem probe we're guaranteed to only get the
> >> > ports of the modem, so we wouldn't need to do any relationship walking
> >> > right?
> >>
> >> The relationship walking is coming from e.g. the QMI and MBIM
> >> implementations. They both do the same stuff when connecting, looking
> >> for which is the correct control port to use with a given interface.
> >> In QMI modems, for example, you may get several WWAN interfaces and
> >> several QMI ports; and the relationship is 1-to-1 there; you cannot
> >> use any QMI port for any WWAN.
> >>
> >> In the case of the Huawei driver in the kernel, the logic is likely
> >> the same, there is a direct relationship between the /dev/cdc-wdm port
> >> and the network interface being managed, so I wouldn't remove those
> >> checks.
> >>
> >> What we can do, though, is to improve the logic (not only here, but
> >> also for QMI and MBIM) so that the check of which control port
> >> corresponds to which net interface is done once during bootup. I'll do
> >> that, but out of this branch for now.
> >
> > How about the attached patch?  It also includes some rebase-to-master
> > fixups since this branch was pre s/MMAtSerialPort/MMPortSerialAt but I
> > think the idea holds?
> 
> 
> I have now updated the "aleksander/huawei-ndisdup-cdc-wdm-rebased-1.2"
> branch applying more or less your logic (re-rebased, so please
> re-fetch a fresh one). Instead of setting a huawei-specific tag in the
> port object, I used a generic "parent-path" property which is set for
> every MMPort. Along with the huawei-specific logic change to look for
> the matching /dev/cdc-wdm port, I also modified the QMI and MBIM
> implementations to use the same approach.
> 
> Let me know what you think,

"port: store parent sysfs path in each MMPort" is missing a "g_free
(self->priv->parent_path);" in finalize().

The rest looks good to me.

Dan