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

[Aleksander Morgado]

On Mon, Jun 23, 2014 at 5:49 PM, Dan Williams <dcbw at redhat.com> wrote:
> 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.
>

Ah, nice catch :)

Fixed that, and merged the branch to git master now.

-- 
Aleksander
https://aleksander.es