Re: [PATCH v3 2/4] bus: fsl-mc: add restool userspace support

Re: [PATCH v3 2/4] bus: fsl-mc: add restool userspace support

[Arnd Bergmann]

On Wed, Mar 28, 2018 at 4:27 PM, Ioana Ciornei <ioana.ciornei@nxp.com> wrote:
> Hi,
>
>>
>> Hi Ioana,
>>
>> So this driver is a direct passthrough to your hardware for passing fixed-
>> length command/response pairs. Have you considered using a higher-level
>> interface instead?
>>
>> Can you list some of the commands that are passed here as clarification, and
>> explain what the tradeoffs are that have led to adopting a low-level interface
>> instead of a high-level interface?
>>
>> The main downside of the direct passthrough obviously is that you tie your
>> user space to a particular hardware implementation, while a high-level
>> abstraction could in principle work across a wider range of hardware revisions
>> or even across multiple vendors implementing the same concept by different
>> means.
>
> If by "higher-level" you mean an implementation where commands are created by the kernel at userspace's request, then I believe this approach is not really viable because of the sheer number of possible commands that would bloat the driver.
>
> For example, a DPNI (Data Path Network Interface) can be created using a command that has the following structure:
>
> struct dpni_cmd_create {
>         uint32_t options;
>         uint8_t num_queues;
>         uint8_t num_tcs;
>         uint8_t mac_filter_entries;
>         uint8_t pad1;
>         uint8_t vlan_filter_entries;
>         uint8_t pad2;
>         uint8_t qos_entries;
>         uint8_t pad3;
>         uint16_t fs_entries;
> };
>
> In the above structure, each field has a meaning that the end-user might want to be able to change according to their particular use-case (not much is left at its default value).
> The same level of complexity is encountered for all the commands that interact with Data Path objects such as DPBP(buffer pools), DPRC(Resource Container) etc.
> You can find more examples of commands in restool's repo: https://github.com/qoriq-open-source/restool/tree/integration/mc_v10
>
> In my opinion, an in-kernel implementation that is equivalent in terms of flexibility will turn
> into a giant ioctl parser, all while also exposing an userspace API that is not as simple/easy to use.

(adding the netdev list)

The command you list there seems to be networking related, so instead of
an ioctl based interface, a high-lever interface would likely use netlink
for consistency with other drivers. Are all commands for networking
or are there some that are talking to the device to do something unrelated?

Obviously creating a high-level interface would be a lot of work in the kernel,
and it only pays off if there are multiple independent users, we wouldn't do
that for just one driver.

I'm still not convinced either way (high-level or low-level
interface), but I think
this needs to be discussed with the networking maintainers. Given the examples
on the github page you linked to, the high-level user space commands
based on these ioctls

   ls-addni   # adds a network interface
   ls-addmux  # adds a dpdmux
   ls-addsw   # adds an l2switch
   ls-listmac # lists MACs and their connections
   ls-listni  # lists network interfaces and their connections

and I see that you also support the switchdev interface in
drivers/staging/fsl-dpaa2, which I think does some of the same
things, presumably by implementing the switchdev API using
fsl_mc_command low-level interfaces in the kernel.

Is that a correct interpretation? If yes, could we extend switchdev
or other networking interfaces to fill in whatever those don't handle
yet?

>> > @@ -14,10 +14,18 @@
>> >   * struct fsl_mc_command - Management Complex (MC) command
>> structure
>> >   * @header: MC command header
>> >   * @params: MC command parameters
>> > + *
>> > + * Used by RESTOOL_SEND_MC_COMMAND
>> >   */
>> >  struct fsl_mc_command {
>> >         __u64 header;
>> >         __u64 params[MC_CMD_NUM_OF_PARAMS];  };
>> >
>> > +#define RESTOOL_IOCTL_TYPE     'R'
>> > +#define RESTOOL_IOCTL_SEQ      0xE0
>>
>> I tried to follow the code path into the hardware and am a bit confused about
>> the semantics of the 'header' field and the data. Both are accessed passed to
>> the hardware using
>>
>>        writeq(le64_to_cpu(cmd->header))
>>
>> which would indicate a fixed byte layout on the user structure and that it
>> should really be a '__le64' instead of '__u64', or possibly should be
>> represented as '__u8 header[8]' to clarify that the byte ordering is supposed
>> to match the order of the byte addresses of the register.
>>
>
> Indeed, the struct fsl_mc_command should use '__le64' instead of '__u64', so that the UAPI header file clearly states what endianness should the userspace prepare.
> The writeq appeared as a consequence of a previous mail thread https://lkml.org/lkml/2017/7/17/415, because the endianness of the command is handled by the user and the bus should leave the binary blob intact.

Ok. However, with the dpni_cmd_create structure you list above, it
seems that it doesn't actually contain a set of __le64 but rather
an array of bytes, so it might be best to

>> However, the in-kernel usage of that field suggests that we treat it as a 64-bit
>> cpu-endian number, for which the hardware needs to know the endianess of
>> the currently running kernel and user space.
>>
>
> I might have not understood what you are trying to say, but the in-kernel code treats the fsl_mc_command as little endian by using cpu_to_leXX and leXX_to_cpu in order to setup the command structure.

One function that seems to have a problem is this one:

static enum mc_cmd_status mc_cmd_hdr_read_status(struct fsl_mc_command *cmd)
{
        struct mc_cmd_header *hdr = (struct mc_cmd_header *)&cmd->header;

        return (enum mc_cmd_status)hdr->status;
}

If cmd->header is little-endian here, then the result is not
an 'enum mc_cmd_status'. I think it would be good to change the
types for fsl_mc_command to __le64 first and run everything through
'sparse' with 'make C=1' to find any remaining endianess problems.

        Arnd

Re: [PATCH v3 2/4] bus: fsl-mc: add restool userspace support

[Andrew Lunn]

> I'm still not convinced either way (high-level or low-level
> interface), but I think
> this needs to be discussed with the networking maintainers. Given the examples
> on the github page you linked to, the high-level user space commands
> based on these ioctls
> 
>    ls-addni   # adds a network interface
>    ls-addmux  # adds a dpdmux
>    ls-addsw   # adds an l2switch
>    ls-listmac # lists MACs and their connections
>    ls-listni  # lists network interfaces and their connections
> 
> and I see that you also support the switchdev interface in
> drivers/staging/fsl-dpaa2, which I think does some of the same
> things, presumably by implementing the switchdev API using
> fsl_mc_command low-level interfaces in the kernel.

Hi Arnd

I agree that switchdev and devlink should be the correct way to handle
this. The low level plumbing of the hardware should all be
hidden. There should not be any user space commands needed other than
the usual network configuration tools and devlink.

      Andrew

RE: [PATCH v3 2/4] bus: fsl-mc: add restool userspace support

[Ioana Ciornei]

> 
> > I'm still not convinced either way (high-level or low-level
> > interface), but I think this needs to be discussed with the networking
> > maintainers. Given the examples on the github page you linked to, the
> > high-level user space commands based on these ioctls
> >
> >    ls-addni   # adds a network interface
> >    ls-addmux  # adds a dpdmux
> >    ls-addsw   # adds an l2switch
> >    ls-listmac # lists MACs and their connections
> >    ls-listni  # lists network interfaces and their connections
> >
> > and I see that you also support the switchdev interface in
> > drivers/staging/fsl-dpaa2, which I think does some of the same things,
> > presumably by implementing the switchdev API using fsl_mc_command
> > low-level interfaces in the kernel.
> 
> Hi Arnd
> 
> I agree that switchdev and devlink should be the correct way to handle this. The
> low level plumbing of the hardware should all be hidden. There should not be
> any user space commands needed other than the usual network configuration
> tools and devlink.
> 

Hi,

The commands listed above are for creating/destroying DPAA2 objects in Management Complex and not for runtime configuration where standard userspace tools are used.
Restool is responsible for creating objects in Management complex and this process can be seen as the equivalent of hotplugging a peripheral rather than configuring it, thus there is no standard userspace tool to handle that.

* The Management Complex is configured to create a specific set of DPAA2 objects dynamically through Restool (by sending create commands) or statically, at boot time, through a configuration file (Data Path Layout file)
* The objects are then probed and configured by the corresponding drivers
* The objects are controlled at runtime by the user via standard tools (e.g. ethtool for network interfaces).
  
I hope this gives a better understanding on the DPAA2 hardware and software architecture. The fsl-mc bus documentation gives more details on this: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git/tree/Documentation/networking/dpaa2/overview.rst?h=staging-next

Ioana

Re: [PATCH v3 2/4] bus: fsl-mc: add restool userspace support

[Andrew Lunn]

Hi Ioana

> The commands listed above are for creating/destroying DPAA2 objects
> in Management Complex and not for runtime configuration where
> standard userspace tools are used.

Please can you explain why this is not just plumbing inside a
switchdev driver?

The hardware has a number of physical ports. So on probe, i would
expect it to create a DPMAC, DPNI, and DPIO for each port, and a linux
netdev. From then on, standard tools are all that are needed. The
switchdev driver can create a l2 switch object when the user uses the
ip link add name br0 type bridge. It can then connect the switch
object to the DPNI when the user adds an interface to the switch, etc.

       Andrew

RE: [PATCH v3 2/4] bus: fsl-mc: add restool userspace support

[Razvan Stefanescu]

Hello Andrew,

> -----Original Message-----
> From: Andrew Lunn [mailto:andrew@lunn.ch]
> Sent: Monday, April 2, 2018 4:45 PM
> To: Ioana Ciornei <ioana.ciornei@nxp.com>
> Cc: Arnd Bergmann <arnd@arndb.de>; gregkh
> <gregkh@linuxfoundation.org>; Laurentiu Tudor
> <laurentiu.tudor@nxp.com>; Linux Kernel Mailing List <linux-
> kernel@vger.kernel.org>; Stuart Yoder <stuyoder@gmail.com>; Ruxandra
> Ioana Ciocoi Radulescu <ruxandra.radulescu@nxp.com>; Razvan Stefanescu
> <razvan.stefanescu@nxp.com>; Roy Pledge <roy.pledge@nxp.com>;
> Networking <netdev@vger.kernel.org>
> Subject: Re: [PATCH v3 2/4] bus: fsl-mc: add restool userspace support
> 
> Hi Ioana
> 
> > The commands listed above are for creating/destroying DPAA2 objects
> > in Management Complex and not for runtime configuration where
> > standard userspace tools are used.
> 
> Please can you explain why this is not just plumbing inside a
> switchdev driver?
> 
> The hardware has a number of physical ports. So on probe, i would
> expect it to create a DPMAC, DPNI, and DPIO for each port, and a linux
> netdev. From then on, standard tools are all that are needed. The
> switchdev driver can create a l2 switch object when the user uses the
> ip link add name br0 type bridge. It can then connect the switch
> object to the DPNI when the user adds an interface to the switch, etc.
> 

I'll chime in as you mentioned switchdev driver. 

DPAA2 offers several object-based abstractions for modeling network
related devices (interfaces, L2 Ethernet switch) or accelerators
(DPSECI - crypto and DPDCEI - compression), the latter not up-streamed yet.
They are modeled using various low-level resources (e.g. queues,
classification tables, physical ports) and have multiple configuration and
interconnectivity options, managed by the Management Complex. 
Resources are limited and they are only used when needed by the objects,
to accommodate more configurations and usage scenarios.
 
Some of the objects have a 1-to-1 correspondence to physical resources
(e.g. DPMACs to physical ports), while others (like DPNIs and DPSW)
can be seen as a collection of the mentioned resources. The types and 
number of such objects are not predetermined.

When the board boots up, none of them exist yet. Restool allows a user to
define the system topology, by providing a way to dynamically create, destroy
and interconnect these objects.

After an object is created, it will be presented on the fsl-mc bus. A driver
is loaded to implement the required kernel interfaces specific to that object
type. Kernel can boot and afterwards the DPAA2 objects are added, as the user
requires.

As you mentioned DPMACs: objects of this type can be connected only to a DPNI
(a network interface like object) or to a DPSW (L2 ethernet switch) port.
Likewise, a DPNI can have only one connection (to a DPMAC, a DPSW port or
another DPNI object).

Here's several examples of valid connection types:
  * DPMAC <----> DPNI (standard network i/f corresponding to a physical port)
  * DPMAC <----> DPSW (physical port in a switch)
  * DPNI <----> DPSW (virtual network interface connected to a switch port)
  * DPNI <----> DPNI

In the latter case, the two DPNIs will not be connected to any physical
port, but can be used as a point-to-point connection between two virtual
machines for instance.

So, it is not possible to connect a DPNI to a DPSW after it was connected
to a DPMAC. The DPNI-DPMAC pair would have to be disconnected and
DPMAC will be reconnected to the switch. DPNI interface that is no longer
connected to a DPMAC will be destroyed and any new addition/deletion of
a DPNI/DPMAC interface to the switch port will trigger the entire switch
re-configuration.

Best regards,
Razvan Stefanescu

Re: [PATCH v3 2/4] bus: fsl-mc: add restool userspace support

[Andrew Lunn]

On Tue, Apr 03, 2018 at 11:12:52AM +0000, Razvan Stefanescu wrote:
> DPAA2 offers several object-based abstractions for modeling network
> related devices (interfaces, L2 Ethernet switch) or accelerators
> (DPSECI - crypto and DPDCEI - compression), the latter not up-streamed yet.
> They are modeled using various low-level resources (e.g. queues,
> classification tables, physical ports) and have multiple configuration and
> interconnectivity options, managed by the Management Complex. 
> Resources are limited and they are only used when needed by the objects,
> to accommodate more configurations and usage scenarios.
>  
> Some of the objects have a 1-to-1 correspondence to physical resources
> (e.g. DPMACs to physical ports), while others (like DPNIs and DPSW)
> can be seen as a collection of the mentioned resources. The types and 
> number of such objects are not predetermined.
> 
> When the board boots up, none of them exist yet. Restool allows a user to
> define the system topology, by providing a way to dynamically create, destroy
> and interconnect these objects.

Hi Razvan

The core concept with Linux networking and offload is that the
hardware is there to accelerate what Linux can already do. Since Linux
can already do it, i don't need any additional tools.

You have new hardware. It might offer features which we currently
don't have offload support for. But all the means is you need to
extend the core networking code which implements the software version
of that feature to offload to the hardware.

The board knows how many physical ports it has. switchdev can then
setup the plumbing to create the objects needed to represent the
ports. Restool is not needed for that.

> In the latter case, the two DPNIs will not be connected to any physical
> port, but can be used as a point-to-point connection between two virtual
> machines for instance.
 
Can Linux already do this? Isn't that what PCI Virtual Functions are
all about? You need to find the current Linux concept for this, and
extend it to offload the functionality to hardware. If Linux can do
it, it already has the tools to configure it. Restool is not needed
for that.

> So, it is not possible to connect a DPNI to a DPSW after it was
> connected to a DPMAC. The DPNI-DPMAC pair would have to be
> disconnected and DPMAC will be reconnected to the switch. DPNI
> interface that is no longer connected to a DPMAC will be destroyed
> and any new addition/deletion of a DPNI/DPMAC interface to the
> switch port will trigger the entire switch re-configuration.

Switches and ports connected to switches are dynamic. They come and
go. You don't expect it to happen very often, but Linux has no
restrictions on this. You need to figure out how best to offload this
to your hardware. Maybe when you create the switch object you make a
guess as to how many ports you need. Leave some of the ports not
connected to anything. You can then add ports to the switch using the
free ports. If you run out of ports, you have no choice but to destroy
the switch object and create a new one. Hopefully that does not take
too long. Restool is not needed for this, it all happens within the
switchdev driver.

	  Andrew

Re: [PATCH v3 2/4] bus: fsl-mc: add restool userspace support

[Stuart Yoder]

On Wed, Mar 28, 2018 at 10:43 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Wed, Mar 28, 2018 at 4:27 PM, Ioana Ciornei <ioana.ciornei@nxp.com> wrote:
>> Hi,
>>
>>>
>>> Hi Ioana,
>>>
>>> So this driver is a direct passthrough to your hardware for passing fixed-
>>> length command/response pairs. Have you considered using a higher-level
>>> interface instead?
>>>
>>> Can you list some of the commands that are passed here as clarification, and
>>> explain what the tradeoffs are that have led to adopting a low-level interface
>>> instead of a high-level interface?
>>>
>>> The main downside of the direct passthrough obviously is that you tie your
>>> user space to a particular hardware implementation, while a high-level
>>> abstraction could in principle work across a wider range of hardware revisions
>>> or even across multiple vendors implementing the same concept by different
>>> means.
>>
>> If by "higher-level" you mean an implementation where commands are created by the kernel at userspace's request, then I believe this approach is not really viable because of the sheer number of possible commands that would bloat the driver.
>>
>> For example, a DPNI (Data Path Network Interface) can be created using a command that has the following structure:
>>
>> struct dpni_cmd_create {
>>         uint32_t options;
>>         uint8_t num_queues;
>>         uint8_t num_tcs;
>>         uint8_t mac_filter_entries;
>>         uint8_t pad1;
>>         uint8_t vlan_filter_entries;
>>         uint8_t pad2;
>>         uint8_t qos_entries;
>>         uint8_t pad3;
>>         uint16_t fs_entries;
>> };
>>
>> In the above structure, each field has a meaning that the end-user might want to be able to change according to their particular use-case (not much is left at its default value).
>> The same level of complexity is encountered for all the commands that interact with Data Path objects such as DPBP(buffer pools), DPRC(Resource Container) etc.
>> You can find more examples of commands in restool's repo: https://github.com/qoriq-open-source/restool/tree/integration/mc_v10
>>
>> In my opinion, an in-kernel implementation that is equivalent in terms of flexibility will turn
>> into a giant ioctl parser, all while also exposing an userspace API that is not as simple/easy to use.
>
> (adding the netdev list)
>
> The command you list there seems to be networking related, so instead of
> an ioctl based interface, a high-lever interface would likely use netlink
> for consistency with other drivers. Are all commands for networking
> or are there some that are talking to the device to do something unrelated?
>
> Obviously creating a high-level interface would be a lot of work in the kernel,
> and it only pays off if there are multiple independent users, we wouldn't do
> that for just one driver.
>
> I'm still not convinced either way (high-level or low-level
> interface), but I think
> this needs to be discussed with the networking maintainers. Given the examples
> on the github page you linked to, the high-level user space commands
> based on these ioctls
>
>    ls-addni   # adds a network interface
>    ls-addmux  # adds a dpdmux
>    ls-addsw   # adds an l2switch
>    ls-listmac # lists MACs and their connections
>    ls-listni  # lists network interfaces and their connections
>
> and I see that you also support the switchdev interface in
> drivers/staging/fsl-dpaa2, which I think does some of the same
> things, presumably by implementing the switchdev API using
> fsl_mc_command low-level interfaces in the kernel.
>
> Is that a correct interpretation? If yes, could we extend switchdev
> or other networking interfaces to fill in whatever those don't handle
> yet?

The wrapper scripts you referenced are not sufficient to show the scope
of what the proposed user space interface is for.  The command list is
not just about networking related objects, as there are quite a few
other types of  objects as well:
    dprc - container object representing an fsl-mc bus instance...i.e. other
           objects are attached to this bus
    dpio - used for queuing operations towards any accelerator or network
           interface
    dpbp - buffer pool object
    dpmcp - command portal interface
    dpdmai - DMA engine
    dpseci - crypto accelerator
    dpdcei - compression/decompression accelerator
    dpni - network interface
    dprtc - real time counter
    dpaiop - heterogenous core complex for packet processing offload
    dpmac - represents an Ethernet MAC
    dpsw - network switch
    dpcon - network concentrator
    dpci - communication interface

The proposed ioctl interface is about:
   A)  creating and destroying all those object types
   B)  managing the dprc containers they live in, including moving
       objects between containers
   C)  where applicable establishing connections between different
       objects

No _operational_ aspects of these object/devices is being handled
through this interface.  The network interface should be managed
through ethtool, for example.  The proposed ioctl interface is about
bringing the devices into existence and getting them "wired" up.

Suppose you want to create and assign a network interface to a KVM
virtual machine, you would do something like the following using
a user space tool like restool:
   -create a new (empty) dprc object
   -create a new dpni and assign it to the dprc
   -create a new dpio and assign it to the dprc
   -create a new dpbp and assign it to the dprc
   -create a new dpmcp and assign it to the dprc
   -create a new dpmac and assign it to the dprc
   -connect the dpni to the dpmac

Now, at this point you have a functional set of objects that
can function as a network interface.

That dprc can now be assigned to a KVM VM using vfio and the
guest will see a dprc that it can probe and enumerate using the
fsl-mc bus infrastructure that is now upstream.

There is no existing kernel <--> user space mechanism that will
work to do all that, so something new was needed.

As far as low-level vs high-level...we did consider a higher level
interface that would expose operations on individual object such
as "create dpbp", but the user space API gets complex and
fragile for no obvious value.  Every object needs commands to
create/destroy and get attributes.  There is a sizeable dprc command
set.  Every time an object is enhanced (with a corresponding major
or minor version rev) you have to change the ioctl interface.

Having a simple command passthrough interface reduces complexity
in the kernel and provides an interface that should be very stable.

The fsl-mc bus and DPAA2 is very NXP-specific, so there doesn't
seem to be anything that can be made generic here to provide
more common benefit.

Thanks,
Stuart

Re: [PATCH v3 2/4] bus: fsl-mc: add restool userspace support

[Andrew Lunn]

> Suppose you want to create and assign a network interface to a KVM
> virtual machine, you would do something like the following using
> a user space tool like restool:
>    -create a new (empty) dprc object
>    -create a new dpni and assign it to the dprc
>    -create a new dpio and assign it to the dprc
>    -create a new dpbp and assign it to the dprc
>    -create a new dpmcp and assign it to the dprc
>    -create a new dpmac and assign it to the dprc
>    -connect the dpni to the dpmac

Hi Stuart

It this connecting to a physical port at the bottom?

If so, i would expect that when you probe the device you just create
all these for each physical port. You then just need to map one of
them into the KVM, in the same way you map one PCI device into a KVM.

If these are virtual devices, VF devices you would normally do

echo 4 > /sys/class/net/<device name>/device/sriov_numvfs

on the physical device to create virtual devices.

> The fsl-mc bus and DPAA2 is very NXP-specific, so there doesn't
> seem to be anything that can be made generic here to provide
> more common benefit.

Which is why you should try to avoid all of this.  The user knows how
to use standard linux commands and concepts. They don't want to have
to learn the inside plumbing of your hardware.

       Andrew

Re: [PATCH v3 2/4] bus: fsl-mc: add restool userspace support

[Stuart Yoder]

On Tue, Apr 3, 2018 at 8:05 PM, Andrew Lunn <andrew@lunn.ch> wrote:
>> Suppose you want to create and assign a network interface to a KVM
>> virtual machine, you would do something like the following using
>> a user space tool like restool:
>>    -create a new (empty) dprc object
>>    -create a new dpni and assign it to the dprc
>>    -create a new dpio and assign it to the dprc
>>    -create a new dpbp and assign it to the dprc
>>    -create a new dpmcp and assign it to the dprc
>>    -create a new dpmac and assign it to the dprc
>>    -connect the dpni to the dpmac
>
> Hi Stuart
>
> It this connecting to a physical port at the bottom?

Yes.

> If so, i would expect that when you probe the device you just create
> all these for each physical port.

The problem is that there is not just one set of objects to implement a network
interface.  For the highest throughput packet processing you need one dpio
per core.  So, it will depend on what the requirements are.  You might want
multiple dpbp (buffer pools) and set up pools of different size
buffers for different
packet classifications.

You might want to have other objects like a crypto accelerator (dpseci) in the
container as well.

The dprc is a container holding any combination of those objects.  So you have
complete flexibility.

> You then just need to map one of
> them into the KVM, in the same way you map one PCI device into a KVM.
>
> If these are virtual devices, VF devices you would normally do
>
> echo 4 > /sys/class/net/<device name>/device/sriov_numvfs
>
> on the physical device to create virtual devices.
>
>> The fsl-mc bus and DPAA2 is very NXP-specific, so there doesn't
>> seem to be anything that can be made generic here to provide
>> more common benefit.
>
> Which is why you should try to avoid all of this.  The user knows how
> to use standard linux commands and concepts. They don't want to have
> to learn the inside plumbing of your hardware.

I hear you.  It is more complicated this way...having all these individual
objects vs just a single "bundle" of them that represents a NIC.  But, that's
the way the DPAA2 hardware is, and we're implementing kernel support for
the hardware as it is.

Thanks,
Stuart

Re: [PATCH v3 2/4] bus: fsl-mc: add restool userspace support

[Andrew Lunn]

> I hear you.  It is more complicated this way...having all these individual
> objects vs just a single "bundle" of them that represents a NIC.  But, that's
> the way the DPAA2 hardware is, and we're implementing kernel support for
> the hardware as it is.

Hi Stuart

I see we are not making any progress here.

So what i suggest is you post the kernel code and configuration tool
concept to netdev for a full review. You want reviews from David
Miller, Jiri Pirko, Jakub Kicinski, David Ahern, etc.

	Andrew

Re: [PATCH v3 2/4] bus: fsl-mc: add restool userspace support

[Stuart Yoder]

On Wed, Apr 4, 2018 at 7:42 AM, Andrew Lunn <andrew@lunn.ch> wrote:
>> I hear you.  It is more complicated this way...having all these individual
>> objects vs just a single "bundle" of them that represents a NIC.  But, that's
>> the way the DPAA2 hardware is, and we're implementing kernel support for
>> the hardware as it is.
>
> Hi Stuart
>
> I see we are not making any progress here.
>
> So what i suggest is you post the kernel code and configuration tool
> concept to netdev for a full review. You want reviews from David
> Miller, Jiri Pirko, Jakub Kicinski, David Ahern, etc.

I know Ioana has other feedback she is addressing so a respin will be coming
soon, and she can include those additional reviewers.

Thanks,
Stuart

Re: [PATCH v3 2/4] bus: fsl-mc: add restool userspace support

[Laurentiu Tudor]

Hello,

My 2c below.

On 04/04/2018 03:42 PM, Andrew Lunn wrote:
>> I hear you.  It is more complicated this way...having all these individual
>> objects vs just a single "bundle" of them that represents a NIC.  But, that's
>> the way the DPAA2 hardware is, and we're implementing kernel support for
>> the hardware as it is.
>
> Hi Stuart
>
> I see we are not making any progress here.
>
> So what i suggest is you post the kernel code and configuration tool
> concept to netdev for a full review. You want reviews from David
> Miller, Jiri Pirko, Jakub Kicinski, David Ahern, etc.
>

I think that the discussion steered too much towards networking related 
topics, while this ioctl doesn't have much to do with networking.
It's just an ioctl for our mc-bus bus driver that is used to manage the 
devices on this bus through userspace tools.
In addition, I'd drop any mention of our reference user space app 
(restool) to emphasize that this ioctl is not added just for a 
particular user space app. I think Stuart also mentioned this.

---
Thanks & Best Regards, Laurentiu

Re: [PATCH v3 2/4] bus: fsl-mc: add restool userspace support

[Andrew Lunn]

On Thu, Apr 05, 2018 at 10:30:01AM +0000, Laurentiu Tudor wrote:
> Hello,
> 
> My 2c below.
> 
> On 04/04/2018 03:42 PM, Andrew Lunn wrote:
> >> I hear you.  It is more complicated this way...having all these individual
> >> objects vs just a single "bundle" of them that represents a NIC.  But, that's
> >> the way the DPAA2 hardware is, and we're implementing kernel support for
> >> the hardware as it is.
> >
> > Hi Stuart
> >
> > I see we are not making any progress here.
> >
> > So what i suggest is you post the kernel code and configuration tool
> > concept to netdev for a full review. You want reviews from David
> > Miller, Jiri Pirko, Jakub Kicinski, David Ahern, etc.
> >
> 
> I think that the discussion steered too much towards networking related 
> topics, while this ioctl doesn't have much to do with networking.

Hi Laurentiu

So i can use switchdev without it? I can modprobe the switchdev
driver, all the physical interfaces will appear, and i can use ip addr
add etc. I do not need to use a user space tool at all in order to use
the network functionality?

    Andrew

Re: [PATCH v3 2/4] bus: fsl-mc: add restool userspace support

[Laurentiu Tudor]

On 04/05/2018 02:47 PM, Andrew Lunn wrote:
> On Thu, Apr 05, 2018 at 10:30:01AM +0000, Laurentiu Tudor wrote:
>> Hello,
>>
>> My 2c below.
>>
>> On 04/04/2018 03:42 PM, Andrew Lunn wrote:
>>>> I hear you.  It is more complicated this way...having all these individual
>>>> objects vs just a single "bundle" of them that represents a NIC.  But, that's
>>>> the way the DPAA2 hardware is, and we're implementing kernel support for
>>>> the hardware as it is.
>>>
>>> Hi Stuart
>>>
>>> I see we are not making any progress here.
>>>
>>> So what i suggest is you post the kernel code and configuration tool
>>> concept to netdev for a full review. You want reviews from David
>>> Miller, Jiri Pirko, Jakub Kicinski, David Ahern, etc.
>>>
>>
>> I think that the discussion steered too much towards networking related
>> topics, while this ioctl doesn't have much to do with networking.
>
> Hi Laurentiu
>
> So i can use switchdev without it? I can modprobe the switchdev
> driver, all the physical interfaces will appear, and i can use ip addr
> add etc. I do not need to use a user space tool at all in order to use
> the network functionality?

Absolutely!
In normal use cases the system designer, depending on the requirements, 
configures the various devices that it desires through a firmware 
configuration (think something like a device tree). The devices 
configured are presented on the mc-bus and probed normally by the 
kernel. The standard networking linux tools can be used as expected.

The ioctl is necessary only for more advanced use cases that are 
supported by this bus. Think "more dynamic" scenarios that involve 
linking & unlinking various devices at runtime, maybe some 
virtualization scenarios. Unfortunately I'm not the architect type of 
guy so I don't have more specific examples to better illustrate ...

---
Best Regards, Laurentiu

Re: [PATCH v3 2/4] bus: fsl-mc: add restool userspace support

[gregkh]

On Thu, Apr 05, 2018 at 10:30:01AM +0000, Laurentiu Tudor wrote:
> Hello,
> 
> My 2c below.
> 
> On 04/04/2018 03:42 PM, Andrew Lunn wrote:
> >> I hear you.  It is more complicated this way...having all these individual
> >> objects vs just a single "bundle" of them that represents a NIC.  But, that's
> >> the way the DPAA2 hardware is, and we're implementing kernel support for
> >> the hardware as it is.
> >
> > Hi Stuart
> >
> > I see we are not making any progress here.
> >
> > So what i suggest is you post the kernel code and configuration tool
> > concept to netdev for a full review. You want reviews from David
> > Miller, Jiri Pirko, Jakub Kicinski, David Ahern, etc.
> >
> 
> I think that the discussion steered too much towards networking related 
> topics, while this ioctl doesn't have much to do with networking.
> It's just an ioctl for our mc-bus bus driver that is used to manage the 
> devices on this bus through userspace tools.
> In addition, I'd drop any mention of our reference user space app 
> (restool) to emphasize that this ioctl is not added just for a 
> particular user space app. I think Stuart also mentioned this.

I'm not going to take a "generic device configuration ioctl" patch
unless it is documented to all exactly what it does, and why it is
there.

thanks,

greg k-h

Re: [PATCH v3 2/4] bus: fsl-mc: add restool userspace support

[Andrew Lunn]

> > Hi Laurentiu
> >
> > So i can use switchdev without it? I can modprobe the switchdev
> > driver, all the physical interfaces will appear, and i can use ip addr
> > add etc. I do not need to use a user space tool at all in order to use
> > the network functionality?
> 
> Absolutely!

Great.

Then the easiest way forwards is to simply drop the IOCTL code for the
moment. Get the basic support for the hardware into the kernel
first. Then come back later to look at dynamic behaviour which needs
some form of configuration.

> In normal use cases the system designer, depending on the requirements, 
> configures the various devices that it desires through a firmware 
> configuration (think something like a device tree). The devices 
> configured are presented on the mc-bus and probed normally by the 
> kernel. The standard networking linux tools can be used as expected.

So what you should probably do is start a discussion on what this
device tree binding looks like. But you need to be careful even
here. Device tree describes the hardware, not how you configure the
hardware. So maybe DT does not actually fit.

	  Andrew

Re: [PATCH v3 2/4] bus: fsl-mc: add restool userspace support

[Laurentiu Tudor]

Hi Greg,

On 04/05/2018 03:30 PM, gregkh wrote:
> On Thu, Apr 05, 2018 at 10:30:01AM +0000, Laurentiu Tudor wrote:
>> Hello,
>>
>> My 2c below.
>>
>> On 04/04/2018 03:42 PM, Andrew Lunn wrote:
>>>> I hear you.  It is more complicated this way...having all these individual
>>>> objects vs just a single "bundle" of them that represents a NIC.  But, that's
>>>> the way the DPAA2 hardware is, and we're implementing kernel support for
>>>> the hardware as it is.
>>>
>>> Hi Stuart
>>>
>>> I see we are not making any progress here.
>>>
>>> So what i suggest is you post the kernel code and configuration tool
>>> concept to netdev for a full review. You want reviews from David
>>> Miller, Jiri Pirko, Jakub Kicinski, David Ahern, etc.
>>>
>>
>> I think that the discussion steered too much towards networking related
>> topics, while this ioctl doesn't have much to do with networking.
>> It's just an ioctl for our mc-bus bus driver that is used to manage the
>> devices on this bus through userspace tools.
>> In addition, I'd drop any mention of our reference user space app
>> (restool) to emphasize that this ioctl is not added just for a
>> particular user space app. I think Stuart also mentioned this.
>
> I'm not going to take a "generic device configuration ioctl" patch
> unless it is documented to all exactly what it does, and why it is
> there.

The ioctl() is just a simple pass-through interface to the firmware.
It passes commands to the firmware and returns the response back to the 
userspace. Thus the ABI used by the firmware applies for this ioctl() 
and it is documented in detail here:

https://www.nxp.com/docs/en/user-guide/DPAA2_UM.pdf

---
Best Regards, Laurentiu

Re: [PATCH v3 2/4] bus: fsl-mc: add restool userspace support

[gregkh]

On Thu, Apr 05, 2018 at 02:09:47PM +0000, Laurentiu Tudor wrote:
> Hi Greg,
> 
> On 04/05/2018 03:30 PM, gregkh wrote:
> > On Thu, Apr 05, 2018 at 10:30:01AM +0000, Laurentiu Tudor wrote:
> >> Hello,
> >>
> >> My 2c below.
> >>
> >> On 04/04/2018 03:42 PM, Andrew Lunn wrote:
> >>>> I hear you.  It is more complicated this way...having all these individual
> >>>> objects vs just a single "bundle" of them that represents a NIC.  But, that's
> >>>> the way the DPAA2 hardware is, and we're implementing kernel support for
> >>>> the hardware as it is.
> >>>
> >>> Hi Stuart
> >>>
> >>> I see we are not making any progress here.
> >>>
> >>> So what i suggest is you post the kernel code and configuration tool
> >>> concept to netdev for a full review. You want reviews from David
> >>> Miller, Jiri Pirko, Jakub Kicinski, David Ahern, etc.
> >>>
> >>
> >> I think that the discussion steered too much towards networking related
> >> topics, while this ioctl doesn't have much to do with networking.
> >> It's just an ioctl for our mc-bus bus driver that is used to manage the
> >> devices on this bus through userspace tools.
> >> In addition, I'd drop any mention of our reference user space app
> >> (restool) to emphasize that this ioctl is not added just for a
> >> particular user space app. I think Stuart also mentioned this.
> >
> > I'm not going to take a "generic device configuration ioctl" patch
> > unless it is documented to all exactly what it does, and why it is
> > there.
> 
> The ioctl() is just a simple pass-through interface to the firmware.

Ah, so a new syscall?  :)

> It passes commands to the firmware and returns the response back to the 
> userspace. Thus the ABI used by the firmware applies for this ioctl() 
> and it is documented in detail here:
> 
> https://www.nxp.com/docs/en/user-guide/DPAA2_UM.pdf

Let's wait on this until people all agree that it's ok to expose this
directly.

thanks,

greg k-h

Re: [PATCH v3 2/4] bus: fsl-mc: add restool userspace support

[Laurentiu Tudor]

Hi Andrew,

On 04/05/2018 03:48 PM, Andrew Lunn wrote:
>>> Hi Laurentiu
>>>
>>> So i can use switchdev without it? I can modprobe the switchdev
>>> driver, all the physical interfaces will appear, and i can use ip addr
>>> add etc. I do not need to use a user space tool at all in order to use
>>> the network functionality?
>>
>> Absolutely!
>
> Great.
>
> Then the easiest way forwards is to simply drop the IOCTL code for the
> moment. Get the basic support for the hardware into the kernel
> first. Then come back later to look at dynamic behaviour which needs
> some form of configuration.

Hmm, not sure I understand. We already have a fully functional ethernet 
driver [1] and a switch driver [2] ...

>> In normal use cases the system designer, depending on the requirements,
>> configures the various devices that it desires through a firmware
>> configuration (think something like a device tree). The devices
>> configured are presented on the mc-bus and probed normally by the
>> kernel. The standard networking linux tools can be used as expected.
>
> So what you should probably do is start a discussion on what this
> device tree binding looks like. But you need to be careful even
> here. Device tree describes the hardware, not how you configure the
> hardware. So maybe DT does not actually fit.

It's not an actual device tree, but a configuration file that happens to 
reuse the DTS format. I guess my analogy with a device tree was not the 
best.
Detailed documentation on the syntax can be found here [3], chapter 22.

[1] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/staging/fsl-dpaa2/ethernet
[2] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/staging/fsl-dpaa2/ethsw
[3] https://www.nxp.com/docs/en/user-guide/DPAA2_UM.pdf

---
Best Regards, Laurentiu

Re: [PATCH v3 2/4] bus: fsl-mc: add restool userspace support

[Andrew Lunn]

On Thu, Apr 05, 2018 at 02:43:29PM +0000, Laurentiu Tudor wrote:
> Hi Andrew,
> 
> On 04/05/2018 03:48 PM, Andrew Lunn wrote:
> >>> Hi Laurentiu
> >>>
> >>> So i can use switchdev without it? I can modprobe the switchdev
> >>> driver, all the physical interfaces will appear, and i can use ip addr
> >>> add etc. I do not need to use a user space tool at all in order to use
> >>> the network functionality?
> >>
> >> Absolutely!
> >
> > Great.
> >
> > Then the easiest way forwards is to simply drop the IOCTL code for the
> > moment. Get the basic support for the hardware into the kernel
> > first. Then come back later to look at dynamic behaviour which needs
> > some form of configuration.
> 
> Hmm, not sure I understand. We already have a fully functional ethernet 
> driver [1] and a switch driver [2] ...

In staging, the tree of crap. You want to get it out of there and into
the main tree. But that effort is being side lined by the discussion
around this IOCTL call.  The best way forward is to to accept Greg is
not going to take this patchset at the moment, and move on. As you
said, it is not needed for the Ethernet and switchdev driver.

What needs to happen before the Ethernet driver can be reviewed for
moving out of staging?

       Andrew

RE: [PATCH v3 2/4] bus: fsl-mc: add restool userspace support

[Ruxandra Ioana Ciocoi Radulescu]

> -----Original Message-----
> From: Andrew Lunn [mailto:andrew@lunn.ch]
> Sent: Thursday, April 5, 2018 6:24 PM
> To: Laurentiu Tudor <laurentiu.tudor@nxp.com>
> Cc: Stuart Yoder <stuyoder@gmail.com>; Arnd Bergmann <arnd@arndb.de>;
> Ioana Ciornei <ioana.ciornei@nxp.com>; gregkh
> <gregkh@linuxfoundation.org>; Linux Kernel Mailing List <linux-
> kernel@vger.kernel.org>; Ruxandra Ioana Ciocoi Radulescu
> <ruxandra.radulescu@nxp.com>; Razvan Stefanescu
> <razvan.stefanescu@nxp.com>; Roy Pledge <roy.pledge@nxp.com>;
> Networking <netdev@vger.kernel.org>
> Subject: Re: [PATCH v3 2/4] bus: fsl-mc: add restool userspace support
> 
> On Thu, Apr 05, 2018 at 02:43:29PM +0000, Laurentiu Tudor wrote:
> > Hi Andrew,
> >
> > On 04/05/2018 03:48 PM, Andrew Lunn wrote:
> > >>> Hi Laurentiu
> > >>>
> > >>> So i can use switchdev without it? I can modprobe the switchdev
> > >>> driver, all the physical interfaces will appear, and i can use ip addr
> > >>> add etc. I do not need to use a user space tool at all in order to use
> > >>> the network functionality?
> > >>
> > >> Absolutely!
> > >
> > > Great.
> > >
> > > Then the easiest way forwards is to simply drop the IOCTL code for the
> > > moment. Get the basic support for the hardware into the kernel
> > > first. Then come back later to look at dynamic behaviour which needs
> > > some form of configuration.
> >
> > Hmm, not sure I understand. We already have a fully functional ethernet
> > driver [1] and a switch driver [2] ...
> 
> In staging, the tree of crap. You want to get it out of there and into
> the main tree. But that effort is being side lined by the discussion
> around this IOCTL call.  The best way forward is to to accept Greg is
> not going to take this patchset at the moment, and move on. As you
> said, it is not needed for the Ethernet and switchdev driver.
> 
> What needs to happen before the Ethernet driver can be reviewed for
> moving out of staging?

Hi Andrew,

We're waiting for the DPIO driver (which we depend on) to be moved
out of staging first, it's currently under review:
https://lkml.org/lkml/2018/3/27/1086

Ioana

Re: [PATCH v3 2/4] bus: fsl-mc: add restool userspace support

[gregkh]

On Thu, Apr 05, 2018 at 03:35:30PM +0000, Ruxandra Ioana Ciocoi Radulescu wrote:
> > -----Original Message-----
> > From: Andrew Lunn [mailto:andrew@lunn.ch]
> > Sent: Thursday, April 5, 2018 6:24 PM
> > To: Laurentiu Tudor <laurentiu.tudor@nxp.com>
> > Cc: Stuart Yoder <stuyoder@gmail.com>; Arnd Bergmann <arnd@arndb.de>;
> > Ioana Ciornei <ioana.ciornei@nxp.com>; gregkh
> > <gregkh@linuxfoundation.org>; Linux Kernel Mailing List <linux-
> > kernel@vger.kernel.org>; Ruxandra Ioana Ciocoi Radulescu
> > <ruxandra.radulescu@nxp.com>; Razvan Stefanescu
> > <razvan.stefanescu@nxp.com>; Roy Pledge <roy.pledge@nxp.com>;
> > Networking <netdev@vger.kernel.org>
> > Subject: Re: [PATCH v3 2/4] bus: fsl-mc: add restool userspace support
> > 
> > On Thu, Apr 05, 2018 at 02:43:29PM +0000, Laurentiu Tudor wrote:
> > > Hi Andrew,
> > >
> > > On 04/05/2018 03:48 PM, Andrew Lunn wrote:
> > > >>> Hi Laurentiu
> > > >>>
> > > >>> So i can use switchdev without it? I can modprobe the switchdev
> > > >>> driver, all the physical interfaces will appear, and i can use ip addr
> > > >>> add etc. I do not need to use a user space tool at all in order to use
> > > >>> the network functionality?
> > > >>
> > > >> Absolutely!
> > > >
> > > > Great.
> > > >
> > > > Then the easiest way forwards is to simply drop the IOCTL code for the
> > > > moment. Get the basic support for the hardware into the kernel
> > > > first. Then come back later to look at dynamic behaviour which needs
> > > > some form of configuration.
> > >
> > > Hmm, not sure I understand. We already have a fully functional ethernet
> > > driver [1] and a switch driver [2] ...
> > 
> > In staging, the tree of crap. You want to get it out of there and into
> > the main tree. But that effort is being side lined by the discussion
> > around this IOCTL call.  The best way forward is to to accept Greg is
> > not going to take this patchset at the moment, and move on. As you
> > said, it is not needed for the Ethernet and switchdev driver.
> > 
> > What needs to happen before the Ethernet driver can be reviewed for
> > moving out of staging?
> 
> Hi Andrew,
> 
> We're waiting for the DPIO driver (which we depend on) to be moved
> out of staging first, it's currently under review:
> https://lkml.org/lkml/2018/3/27/1086

That's stalled on my side right now as the merge window is open and I
can't do any new stuff until after 4.17-rc1 is out.  So everyone please
be patient a bit...

thanks,

greg k-h

Re: [PATCH v3 2/4] bus: fsl-mc: add restool userspace support

[Andrew Lunn]

> > Hi Andrew,
> > 
> > We're waiting for the DPIO driver (which we depend on) to be moved
> > out of staging first, it's currently under review:
> > https://lkml.org/lkml/2018/3/27/1086
> 
> That's stalled on my side right now as the merge window is open and I
> can't do any new stuff until after 4.17-rc1 is out.  So everyone please
> be patient a bit...

I took a quick look.

There are a few inline functions in .c files which is generally
frowned upon. Let the compiler decide.

e.g:

static inline struct dpaa2_io *service_select_by_cpu(struct dpaa2_io *d,	
                                                     int cpu)
static inline struct dpaa2_io *service_select(struct dpaa2_io *d)

dpaa2_io_down() seems to be too simple. dpaa2_io_create() sets up
interrupt triggers, notifications, and adds the new object to the
dpio_list. dpaa2_io_down() seems to just free the memory. Do
notifications need to be disabled, the object taken off the list?

dpaa2_io_store_create() allocates memory using kzalloc() and then uses
dma_map_single(,,DMA_FROM_DEVICE). The documentation says:

   DMA_FROM_DEVICE synchronisation must be done before the driver
   accesses data that may be changed by the device.  This memory
   should be treated as read-only by the driver.  If the driver needs
   to write to it at any point, it should be DMA_BIDIRECTIONAL (see
   below).

Since it has just been allocated, this seems questionable.

I'm also not sure where the correct call to
dma_map_single(,,DMA_FROM_DEVICE) is?  Should dpaa2_io_store_next()
doing this?

The DMA API usage might need a closer review.

    Andrew