Message-id: <1448621963-32040-1-git-send-email-r.baldyga@samsung.com>
Date:	Fri, 27 Nov 2015 11:58:54 +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,
	Robert Baldyga <r.baldyga@...sung.com>
Subject: [PATCH v2 00/29] usb: gadget: composite: introduce new function API

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 ten patches converts Functions: loopback, sourcesink, ecm, rndis,
hid, acm, eem, ncm, printer and serial to new API. Conversion of another
Functions will be done 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:

v2:
- Addressed comments from Sergei
- Added 6 new patches converting Functions to new API

v1: https://lkml.org/lkml/2015/11/3/288

Robert Baldyga (29):
  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

 drivers/usb/gadget/composite.c             | 947 ++++++++++++++++++++++++++++-
 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_ncm.c        | 320 ++++------
 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/g_zero.h       |   6 +-
 drivers/usb/gadget/legacy/zero.c           |  12 +
 include/linux/usb/composite.h              | 194 ++++++
 15 files changed, 2025 insertions(+), 1884 deletions(-)

-- 
1.9.1

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