Re: [RFC][PATCH 1/2] usb: gadget: configfs: add MTP function

Krzysztof Opasiak
2015-08-13T19:34:54Z

Hello,

On 08/13/2015 09:12 PM, Amit Pundir wrote:
> his MTP function is based on years of work originally done in the
> Android kernel tree by:
> Mike Lockwood<lockwood@android.com>
> Benoit Goby<benoit@android.com>
> Colin Cross<ccross@android.com>
> Arve Hjønnevåg<arve@android.com>
> Peter Oh<poh@broadcom.com>
> Greg Hackmann<ghackmann@google.com>
> Badhri Jagan Sridharan<Badhri@google.com>
> I've folded the series up to make it easier to review, and to provide
> a coherent patch description.
>
> Post Gingerbread (Android v2.3), Android dropped USB Mass Storage
> in favor of Media Transfer Protocal (MTP), which is widely used for
> transferring media files to digital music players and similar
> applications. This USB gadget function implements MTP functionalty.
>
> Historically this function has been a part of Android composite
> gadget driver. Android composite driver was Android's solution
> for dynamic gadget function switching prior to the ConfigFS gadget
> being merged. There were failed few attempts in past
> http://marc.info/?l=linux-usb&m=132451695808552 to upstream Android
> composite driver as well. Now this Android MTP gadget function has been
> re-implemented so as to be used as a generic ConfigFS function instead.
>
> Again, many thanks to Mike, Benoit, Colin, Arve, Peter, Greg and Badhri,
> as they are the real authors of this work. However, I've folded their
> patches together and modified it enough that I don't want them to be
> blamed for any mistakes I've made condensing their patches down.
>
> Cc: Mike Lockwood<lockwood@android.com>
> Cc: Benoit Goby<benoit@android.com>
> Cc: Colin Cross<ccross@android.com>
> Cc: Arve Hjønnevåg<arve@android.com>
> Cc: Peter Oh<poh@broadcom.com>
> Cc: Greg Hackmann<ghackmann@google.com>
> Cc: Badhri Jagan Sridharan<Badhri@google.com>
> Cc: Android Kernel Team<kernel-team@android.com>
> Cc: Greg Kroah-Hartman<gregkh@linuxfoundation.org>
> Cc: Jonathan Corbet<corbet@lwn.net>
> Cc: Felipe Balbi<balbi@ti.com>
> Cc: Andrzej Pietrasiewicz<andrzej.p@samsung.com>
> Cc: Laurent Pinchart<laurent.pinchart@ideasonboard.com>
> Cc: Yegor Yefremov<yegorslists@googlemail.com>
> Cc: Philippe Reynes<tremyfr@gmail.com>
> Cc: John Stultz<john.stultz@linaro.org>
> Cc: Sumit Semwal<sumit.semwal@linaro.org>
> Signed-off-by: Amit Pundir<amit.pundir@linaro.org>

In my humble opinion adding such function to Linux kernel doesn't make
any sense. By design, MTP is a protocol which requires access to
userspace features esp. file system. It is very important to run MTP
daemon with suitable user and LSM label and many many other issues which
should be handled by userspace access policy.

Moreover this is not a fully functional USB function but only some
interface which can be used by mtp-responder (mtp-daemon - call it as
you like) to communicate with host. As we have FunctionFS which allows
to implement any USB function in as a userspace service. As MTP nature
is more related to userspace I think that porting MTP daemon to use this
is a right way to go. This should be much more reasonable than adding
new function which also requires daemon for proper working. So why add
another interface while we can use a generic one?

Best regards,
--
Krzysztof Opasiak
Samsung R&D Institute Poland
Samsung Electronics