Message-id: <56728DAE.6050500@samsung.com>
Date:	Thu, 17 Dec 2015 11:25:50 +0100
From:	Robert Baldyga <r.baldyga@...sung.com>
To:	balbi@...com
Cc:	gregkh@...uxfoundation.org, andrzej.p@...sung.com,
	m.szyprowski@...sung.com, b.zolnierkie@...sung.com,
	linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 00/36] usb: gadget: composite: introduce new function API

Hi Felipe,

I can see that you have applied the documentation fix patch to your
tree. Have also you looked at the remaining patches of this series? What
do you think about this concept? Any comments?

Best regards,
Robert

On 12/11/2015 12:24 PM, Robert Baldyga wrote:
> Hi Felipe,
> 
> Here is my new patch series doing some changes in composite framework
> and modifying USB Function API. Some of concepts changed significantly,
> for example bind process is done automatically inside composite framework
> after collecting descriptors from all Functions. Hence bind() operation
> of USB Function has been replaced with prep_descs(). Besides the other
> benefits, such as simple implementation of gadget-level autoconfig solver,
> changes in API allowed to simplify code of USB Functions, which contain
> lots of boilerplate code.
> 
> First five patches of this series are fixes or improvements, being
> preparation for further changes. Patches from 6 to 19 add implementation
> of new features. Some code allowing coexistence of both old and new API
> is added. This code will be removed after converting all USB Functions
> present in kernel to new API.
> Last 16 patches converts Functions: loopback, sourcesink, ecm, rndis,
> hid, acm, eem, ncm, printer, serial, obex, phonet, ECM subset, uac1,
> uac2 and mass_storage to new API. Conversion of another Functions will
> be provided soon.
> 
> *** What has changed? ***
> 
> The main changes are listed below:
> A. Introduce new descriptors format. It makes descriptors creation process
>    simpler plus creates good place to contain additional information such
>    as result of automatic bind or actually selected altsetting.
> B. Split descriptors creation process into two stages, implemented by
>    two new operations:
>    - prep_descs() provide entity descriptors (interfaces, altsettings
>      and endpoints)
>    - prep_vendor_descs() provide class and vendor specific descriptors.
>    The first one is called before binding funciton to UDC and it's
>    mandatory, because it provides information needed during bind process.
>    The second one is optional and a Function can implement it if it wants
>    to attach some class or vendor specific descriptors. It's called after
>    bind process, so from it's context all information about interface
>    numbers and endpoint addresses is accessible.
> C. Perform bind automatically inside composite framework after collecting
>    descriptors from all USB Functions. Besides removing lots of repetitive
>    code from USB Functions, it gives us two main advantages:
>    - We can have gadget-level autoconfig solver providing better endpoint
>      resources usage. We can choose best endpoint configuration for all
>      Functions in all configurations.
>    - We have composite driver structure creation process separated from
>      bind process which allows to modify configfs to operate directly
>      on composite driver state - both legacy gadgets and configfs can
>      use common composite driver creation process.
>    Function allowing to obtain endpoints after bind process is provided,
>    and it should be called in set_alt().
> D. Replace disable() operation with more powerful clear_alt(). It is
>    called when Function is being disabled or when altsetting being
>    selected on interface which already has active altsetting. It makes
>    API more symmetric, which greatly simplifies resource management.
> E. Handle endpoint enable/disable automatically, which means, that in
>    set_alt() we obtain set of already enabled endpoints for current
>    altsetting. Likewise in clear_alt() endpoints are already disabled.
> F. Change meaning of second parameter of set_alt() operation. Now it
>    contains index of interface within desctiptors array of given USB
>    Function instead of bInterfaceNumber of this interface, which
>    simplifies altsetting handling (so far it was necessary to compare
>    this value with bInterfaceNumber of each interface to find out which
>    altsetting of which interface is being selected).
> G. Handle get_alt() automatically. Currently selected altsetting number
>    is stored for each interface.
> 
> *** How did it work before? ***
> 
> So far USB Functions had to handle bind process manually and deal with
> endpoints state explicitly, which has been making code lengthy and
> bug-prone. USB Functions contained lots of repetitive code which was
> usually copied while creating new USB Function module. This resulted
> with lots of boilerplate code scattered across all Functions present
> in Linux kernel.
> 
> BIND:
> 
> During bind process we had to obtain interface id manually and assign
> it to each interface descriptor (altsetting) of given interface. We also
> had to obtain endpoints manually using usb_ep_autoconfig(). Beside its
> verbosity, this solution resulted with suboptimal endpoints distribution,
> because autoconfig algorithm was aware of requirements of only single
> endpoint at a time.
> 
> udc_bind_to_driver() {
>   composite_bind() {
>     configuration1->bind() {
>       function1->bind() {
>         intf1_id = usb_interface_id(); // Obtain intf id manually
>         ep1 = usb_ep_autoconfig(); // Endpoint-level autoconfig
>         ep2 = usb_ep_autoconfig();
>         intf2_id = usb_interface_id();
>         ep3 = usb_ep_autoconfig();
>         ep4 = usb_ep_autoconfig();
>       }
>       function2->bind() {
> 	intf1_id = usb_interface_id();
>         ep1 = usb_ep_autoconfig();
>         ep2 = usb_ep_autoconfig();
>         ep3 = usb_ep_autoconfig();
>       }
>       function3->bind() {
>         intf1_id = usb_interface_id();
>         ep1 = usb_ep_autoconfig();
>         intf2_id = usb_interface_id();
>         ep2 = usb_ep_autoconfig();
>       }
>     }
>     configuration2->bind() {
>       function1->bind() {
>         intf1_id = usb_interface_id();
>         ep1 = usb_ep_autoconfig();
>       }
>       function2->bind() {
>         intf1_id = usb_interface_id();
>         ep1 = usb_ep_autoconfig();
>       }
>     }
>   }
> }
> 
> SET_ALT:
> 
> In set_alt() we had to guess if any altsetting for given interface had
> been selected before or not. In fact many functions have been doing this
> by storing some information in driver_data field of endpoints or simply
> by doing interface reset regardless of previous state. We also needed
> to guess for which interface set_alt() has been called, because interface
> number passed to this callback was the interface index in configuration,
> not index in function descriptors. It has been making set_alt() handling
> quite problematic.
> 
> function1->set_alt() {
>   id = detect_intf_id();
>   if (id == intf1_id) {
>     intf_specific_disable(); // Disable everything just in case
>     usb_ep_disable(ep1); // Disable endpoint manually
>     usb_ep_disable(ep2);
>     config_ep_by_speed(ep1); // Configure endpoint manually
>     config_ep_by_speed(ep2);
>     usb_ep_enable(ep1); // Enable endpoint manually
>     usb_ep_enable(ep2);
>     intf_specific_enable();
>   } else {
>     intf_specific_disable();
>     usb_ep_disable(ep3);
>     usb_ep_disable(ep4);
>     config_ep_by_speed(ep3);
>     config_ep_by_speed(ep4);
>     usb_ep_enable(ep3);
>     usb_ep_enable(ep4);
>     intf_specific_enable();
>   }
> }
> function2->set_alt() {
>   function_specific_disable();
>   usb_ep_disable(ep1);
>   usb_ep_disable(ep2);
>   config_ep_by_speed(ep1);
>   config_ep_by_speed(ep2);
>   usb_ep_enable(ep1);
>   usb_ep_enable(ep2);
>   function_specific_enable();
> }
> 
> DISABLE:
> 
> The disable() callback has been called only when entire Function had
> being disabled. In this function we had to deactivate all functionality
> and disable all endpoints manually.
> 
> function1->disable() {
>   function_specific_disable();
>   usb_ep_disable(ep1); // Disable endpoint manually
>   usb_ep_disable(ep2);
>   usb_ep_disable(ep3);
>   usb_ep_disable(ep4);
> }
> function2->disable() {
>   function_specific_disable();
>   usb_ep_disable(ep1);
>   usb_ep_disable(ep2);
> }
> 
> *** How does it work now? ***
> 
> BIND:
> 
> Bind process is done entirely by composite framework internals. To
> achieve this, composite needs to have a knowledge about interfaces and
> endpoints which the Function is comprised of. This knowledge is provided
> by prep_descs() callback which assigns descriptors needed for bind process
> to Function. Having all descriptors collected allows to implement
> configuration-level or even gadget-level autoconfig solver. This solver
> could, basing on information from descriptors and endpoint capabilities
> of UDC hardware, which gadget driver is being bound to, distribute
> endpoints over interfaces in much more optimal way than it has been done
> so far. At the end, after binding gadget to UDC hardware,
> prep_vendor_descs() callback is invoked for each Function to allow it
> to provide some class and vendor specific descriptors.
> 
> udc_bind_to_driver() {
>   composite_bind() {
>     configuration1->bind() {
>       function1->prep_descs() { // Gather descriptors
>         usb_function_set_descs(); // Set descs to Function
>       }
>       function2->prep_descs() {
>         usb_function_set_descs();
>       }
>       function3->prep_descs() {
>         usb_function_set_descs();
>       }
>     }
>     usb_config_do_bind(); // Bind entire configuration
> 
>     configuration2->bind() {
>       function1->prep_descs() {
>         usb_function_set_descs();
>       }
>       function2->prep_descs() {
>         usb_function_set_descs();
>       }
>     }
>     usb_config_do_bind();
> 
>     composite_prep_vendor_descs(); // Gather class and vendor descs
>   }
> }
> 
> SET_ALT:
> 
> In set_alt() function we have to obtain endpoints for currently selected
> altsetting. Endpoints are already configured and enabled. Interface number
> passed to set_alt() is its index within Function descriptors array, which
> simplifies set_alt() handling. We also don't need to remember if any
> altsetting has been previously selected, because in such situation
> clear_alt() is called before set_alt(), to allow function to cleanup
> interface state.
> 
> function1->set_alt() {
>   if (intf == 0) {
>     ep1 = usb_function_get_ep();
>     ep2 = usb_function_get_ep();
>     intf_specific_enable();
>   } else {
>     ep3 = usb_function_get_ep();
>     ep4 = usb_function_get_ep();
>     intf_specific_enable();
>   }
> }
> function2->set_alt() {
>   ep1 = usb_function_get_ep();
>   ep2 = usb_function_get_ep();
>   function_specific_enable();
> }
> 
> CLEAR_ALT:
> 
> In clear_alt() callback function can clear interface state and free
> resources allocated in set_alt(). It's called before selecting altsetting
> in interface which has already selected active altsetting, or when
> function is being disabled. Thanks to this clear_alt() can be used as
> an enhanced replacement of disable() operation.
> 
> function1->clear_alt() {
>   if (intf == 0) {
>     intf_specific_disable();
>   } else {
>     intf_specific_disable();
>   }
> }
> function2->clear_alt() {
>   function_specific_disable();
> }
> 
> *** What's next? ***
> 
> The next step is to convert all Functions to new API and cleanup composite
> code. Then it will be possible to implement intelligent configuration-level
> autoconfig solver. We can also try to implement gadget-level autoconfig
> solver which could be capable to reconfigure UDC hardware according to
> requirements of specific gadget driver.
> 
> Thanks to separation of bind process from composite driver creation
> process (adding function to configuration doesn't involve its bind, so
> it can be done before hardware is available), we can also simplify
> configfs gadget implementation. We can make legacy gadgets and configfs
> using common composite driver creation process.
> 
> I believe it's also possible to make descriptors creation process less
> verbose by providing set of macros/functions dedicated for this purpose
> (now descriptors take at average over 200 lines of code per Function).
> 
> I have some WIP version of patches in which I'm doing part of changes
> mentioned above. I can send them as RFC to show what is final result
> which I want to achieve.
> 
> Best regards,
> Robert Baldyga
> 
> Changelog:
> 
> v3:
> - Fixed handling of vendor specific descriptor attached to endpoint
> - Added 6 new patches converting Functions to new API
> 
> v2: https://lkml.org/lkml/2015/11/27/180
> - Addressed comments from Sergei
> - Added 6 new patches converting Functions to new API
> 
> v1: https://lkml.org/lkml/2015/11/3/288
> 
> Robert Baldyga (36):
>   Documentation: usb: update usb-tools repository address
>   usb: gadget: f_sourcesink: make ISO altset user-selectable
>   usb: gadget: f_sourcesink: free requests in sourcesink_disable()
>   usb: gadget: f_loopback: free requests in loopback_disable()
>   usb: gadget: configfs: fix error path
>   usb: gadget: composite: introduce new descriptors format
>   usb: gadget: composite: add functions for descriptors handling
>   usb: gadget: composite: introduce new USB function ops
>   usb: gadget: composite: handle function bind
>   usb: gadget: composite: handle vendor descs
>   usb: gadget: composite: generate old descs for compatibility
>   usb: gadget: composite: disable eps before calling disable() callback
>   usb: gadget: composite: enable eps before calling set_alt() callback
>   usb: gadget: composite: introduce clear_alt() operation
>   usb: gadget: composite: handle get_alt() automatically
>   usb: gadget: composite: add usb_function_get_ep() function
>   usb: gadget: composite: add usb_get_interface_id() function
>   usb: gadget: composite: enable adding USB functions using new API
>   usb: gadget: configfs: add new composite API support
>   usb: gadget: f_loopback: convert to new API
>   usb: gadget: f_sourcesink: convert to new API
>   usb: gadget: f_ecm: conversion to new API
>   usb: gadget: f_rndis: conversion to new API
>   usb: gadget: f_hid: handle requests lifetime properly
>   usb: gadget: f_hid: conversion to new API
>   usb: gadget: f_acm: conversion to new API
>   usb: gadget: f_eem: conversion to new API
>   usb: gadget: f_ncm: conversion to new API
>   usb: gadget: f_printer: conversion to new API
>   usb: gadget: f_serial: conversion to new API
>   usb: gadget: f_obex: conversion to new API
>   usb: gadget: f_phonet: conversion to new API
>   usb: gadget: f_subset: conversion to new API
>   usb: gadget: f_uac1: conversion to new API
>   usb: gadget: f_uac2: conversion to new API
>   usb: gadget: f_mass_storage: conversion to new API
> 
>  Documentation/usb/gadget-testing.txt         |   2 +-
>  drivers/usb/gadget/composite.c               | 950 ++++++++++++++++++++++++++-
>  drivers/usb/gadget/configfs.c                |  24 +-
>  drivers/usb/gadget/function/f_acm.c          | 248 +++----
>  drivers/usb/gadget/function/f_ecm.c          | 321 +++------
>  drivers/usb/gadget/function/f_eem.c          | 154 +----
>  drivers/usb/gadget/function/f_hid.c          | 307 ++++-----
>  drivers/usb/gadget/function/f_loopback.c     | 218 ++----
>  drivers/usb/gadget/function/f_mass_storage.c |  91 +--
>  drivers/usb/gadget/function/f_ncm.c          | 320 +++------
>  drivers/usb/gadget/function/f_obex.c         | 188 ++----
>  drivers/usb/gadget/function/f_phonet.c       | 225 +++----
>  drivers/usb/gadget/function/f_printer.c      | 300 +++------
>  drivers/usb/gadget/function/f_rndis.c        | 321 ++++-----
>  drivers/usb/gadget/function/f_serial.c       | 122 +---
>  drivers/usb/gadget/function/f_sourcesink.c   | 415 +++++-------
>  drivers/usb/gadget/function/f_subset.c       | 165 ++---
>  drivers/usb/gadget/function/f_uac1.c         | 134 ++--
>  drivers/usb/gadget/function/f_uac2.c         | 345 ++++------
>  drivers/usb/gadget/function/g_zero.h         |   6 +-
>  drivers/usb/gadget/function/storage_common.c |  29 -
>  drivers/usb/gadget/function/storage_common.h |   3 -
>  drivers/usb/gadget/legacy/zero.c             |  12 +
>  include/linux/usb/composite.h                | 194 ++++++
>  24 files changed, 2410 insertions(+), 2684 deletions(-)
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists