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

[Dan Williams]

On Fri, 2014-06-13 at 16:38 +0200, Aleksander Morgado wrote:
> On Thu, Jun 12, 2014 at 9:47 PM, Dan Williams <dcbw at redhat.com> wrote:
> >> huawei: use the cdc-wdm port dialing if available
> >
> > g_clear_object() instead of if (foo) g_object_unref (foo)?
> >
> 
> I usually prefer to use g_clear_object() when I explicitly want the
> foo pointer to be re-seted to NULL after the unref(); otherwise I see
> "if(foo) unref(foo)" just cleaner to read... Anyway, I think it may be
> just ok to start switching to using g_clear_object() always, even if
> we're not going to use the pointer again...

Sure no problem.

> > I'd extract the "find dial port" stuff from connect_3gpp() and
> > disconnect_3gpp() into its own function since both code does the same
> > thing.
> >
> 
> Done.
> 
> > Also, a rationale in the commit message would be nice; since I forget
> > why this was necessary.  Does the modem just always want the commands on
> > the cdc-wdm interface if it exists?
> >
> 
> The modem requires the NDISDUP call in that cdc-wdm interface, or it
> won't connect the net port. Added the comment in the commit msg.

Looks good.

> > 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?

Dan
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-fixup-find-ports-better.patch
Type: text/x-patch
Size: 32303 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/modemmanager-devel/attachments/20140619/b7cedf21/attachment-0001.bin>