bus: fsl-mc: enhance Management Complex userspace support

drivers/bus/fsl-mc/fsl-mc-restool.c line 16

Quoted 3 lines

drivers/bus/fsl-mc/fsl-mc-restool.c new file +3 -0

+#include "fsl-mc-private.h"
+
+#define FSL_MC_BUS_MAX_MINORS	1

As you only need/want one character device here, why not just use the misc device api? It's much simpler, and handles all of the housekeeping for you correctly. It also means I don't have to audit all of your chardev code to verify it is correct :)

And it will save you lines-of-code, always a good thing.

thanks,

greg k-h

Quoted 11 lines · Ioana Ciornei

drivers/bus/fsl-mc/fsl-mc-restool.c line 16

Quoted 3 lines

drivers/bus/fsl-mc/fsl-mc-restool.c new file +3 -0

+#include "fsl-mc-private.h"
+
+#define FSL_MC_BUS_MAX_MINORS	1

As you only need/want one character device here, why not just use the misc device api? It's much simpler, and handles all of the housekeeping for you correctly. It also means I don't have to audit all of your chardev code to verify it is correct :)

I have considered the misc device api but the fsl-mc bus, since it is a platform driver, is probing before the misc char driver.

Quoted 12 lines

Quoted 11 lines · Ioana Ciornei

drivers/bus/fsl-mc/fsl-mc-restool.c line 16

Quoted 3 lines

drivers/bus/fsl-mc/fsl-mc-restool.c new file +3 -0

+#include "fsl-mc-private.h"
+
+#define FSL_MC_BUS_MAX_MINORS	1

As you only need/want one character device here, why not just use the misc device api? It's much simpler, and handles all of the housekeeping for you correctly. It also means I don't have to audit all of your chardev code to verify it is correct :)

Quoted 2 lines

I have considered the misc device api but the fsl-mc bus, since it is a platform driver, is probing before the misc char driver.

How? The misc code is just "core code" there's nothing to "probe" here. Is this an init-call ordering issue somehow? A platform driver probe should be after the misc core is initialized. And if not, you can always defer your probe.

thanks,

greg k-h

Adding kernel support for restool, a userspace tool for resource management, means exporting an ioctl capable device file representing the root resource container.
This new functionality in the fsl-mc bus driver intends to provide restool an interface to interact with the MC firmware. Commands that are composed in userspace are sent to the MC firmware through the RESTOOL_SEND_MC_COMMAND ioctl.
By default the implicit MC I/O portal is used for this operation, but if the implicit one is busy, a dynamic portal is allocated and then freed upon execution.

drivers/bus/fsl-mc/fsl-mc-restool.c new file +10 -0

+static long fsl_mc_restool_dev_ioctl(struct file *file,
+                                    unsigned int cmd,
+                                    unsigned long arg)
+{
+       int error;
+
+       switch (cmd) {
+       case RESTOOL_SEND_MC_COMMAND:
+               error = fsl_mc_restool_send_command(arg, file->private_data);
+               break;

@@ -14,10 +14,18 @@
  * struct fsl_mc_command - Management Complex (MC) command structure
  * @header: MC command header
  * @params: MC command parameters

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.

Review line 14

Quoted 2 lines

(quoted diff) +0 -0

@@ -14,10 +14,18 @@
  * struct fsl_mc_command - Management Complex (MC) command

structure

include/uapi/linux/fsl_mc.h line 26

Quoted 11 lines

include/uapi/linux/fsl_mc.h +4 -0

  * @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.

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.

Can you have a look at where the mistake is and what the byteorder for the fsl_mc_command structure is supposed to be? Obviously, this is one thing that would be simplified by using a high-level interface, but it's possible to do it like this as long as it's completely clear how the structure layout is meant to be used in the uapi header.

Arnd

Hi,

Quoted 11 lines

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?

Quoted 5 lines

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.

Quoted 21 lines

Review line 14

Quoted 2 lines

(quoted diff) +0 -0

@@ -14,10 +14,18 @@
  * struct fsl_mc_command - Management Complex (MC) command

structure

include/uapi/linux/fsl_mc.h line 26

Quoted 11 lines

include/uapi/linux/fsl_mc.h +4 -0

  * @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))

Quoted 5 lines

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.

Quoted 4 lines

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.

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.

Quoted 10 lines

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

Quoted 7 lines

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.

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

-----Original Message-----
From: Andrew Lunn [mailto:[email protected]]
Sent: Monday, April 2, 2018 4:45 PM
To: Ioana Ciornei <[email protected]>
Cc: Arnd Bergmann <[email protected]>; gregkh <[email protected]>; Laurentiu Tudor <[email protected]>; Linux Kernel Mailing List <linux- [email protected]>; Stuart Yoder <[email protected]>; Ruxandra Ioana Ciocoi Radulescu <[email protected]>; Razvan Stefanescu <[email protected]>; Roy Pledge <[email protected]>; Networking <[email protected]>
Subject: Re: [PATCH v3 2/4] bus: fsl-mc: add restool userspace support

Hi Ioana

Quoted 3 lines

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.

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.

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.

Quoted 36 lines · Ioana Ciornei

Hi,

Quoted 11 lines

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?

Quoted 5 lines

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

Quoted 6 lines

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?

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

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.

Quoted 10 lines

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.

Quoted 3 lines

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.

Quoted 4 lines

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.

Quoted 4 lines

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.

Hello,

My 2c below.

Quoted 9 lines · Andrew Lunn

Quoted 4 lines

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.

Quoted 4 lines

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.

Quoted 14 lines · Laurentiu Tudor

Hello,

My 2c below.

Quoted 9 lines · Andrew Lunn

Quoted 4 lines

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.

Quoted 7 lines

Quoted 4 lines

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?

Quoted 6 lines

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.

Quoted 7 lines

Quoted 6 lines

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?

Quoted 1 line

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.

Quoted 5 lines

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.

Hi Andrew,

Quoted 11 lines · Andrew Lunn

Quoted 8 lines

Quoted 6 lines

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.

Quoted 4 lines

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

-----Original Message-----
From: Andrew Lunn [mailto:[email protected]]
Sent: Thursday, April 5, 2018 6:24 PM
To: Laurentiu Tudor <[email protected]>
Cc: Stuart Yoder <[email protected]>; Arnd Bergmann <[email protected]>; Ioana Ciornei <[email protected]>; gregkh <[email protected]>; Linux Kernel Mailing List <linux- [email protected]>; Ruxandra Ioana Ciocoi Radulescu <[email protected]>; Razvan Stefanescu <[email protected]>; Roy Pledge <[email protected]>; Networking <[email protected]>
Subject: Re: [PATCH v3 2/4] bus: fsl-mc: add restool userspace support

Quoted 14 lines · Laurentiu Tudor

Hi Andrew,

Quoted 11 lines · Andrew Lunn

Quoted 8 lines

Quoted 6 lines

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.

Quoted 7 lines

Quoted 4 lines

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?

Quoted 42 lines

-----Original Message-----
From: Andrew Lunn [mailto:[email protected]]
Sent: Thursday, April 5, 2018 6:24 PM
To: Laurentiu Tudor <[email protected]>
Cc: Stuart Yoder <[email protected]>; Arnd Bergmann <[email protected]>; Ioana Ciornei <[email protected]>; gregkh <[email protected]>; Linux Kernel Mailing List <linux- [email protected]>; Ruxandra Ioana Ciocoi Radulescu <[email protected]>; Razvan Stefanescu <[email protected]>; Roy Pledge <[email protected]>; Networking <[email protected]>
Subject: Re: [PATCH v3 2/4] bus: fsl-mc: add restool userspace support

Quoted 14 lines · Laurentiu Tudor

Hi Andrew,

Quoted 11 lines · Andrew Lunn

Quoted 8 lines

Quoted 6 lines

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.

Quoted 7 lines

Quoted 4 lines

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.

Quoted 2 lines

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

Quoted 5 lines

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

Hello,

My 2c below.

Quoted 5 lines · Andrew Lunn

Quoted 4 lines

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.

Quoted 8 lines

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.

Quoted 19 lines · Laurentiu Tudor

Hello,

My 2c below.

Quoted 5 lines · Andrew Lunn

Quoted 4 lines

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.

Quoted 8 lines

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.

Quoted 7 lines

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.

Hi Greg,

Quoted 28 lines · gregkh

Quoted 26 lines · Laurentiu Tudor

Hello,

My 2c below.

Quoted 5 lines · Andrew Lunn

Quoted 4 lines

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.

Quoted 8 lines

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.

Quoted 3 lines

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

Introduce the rescan attribute as a device attribute to synchronize the fsl-mc bus objects and the MC firmware.

To rescan the root dprc only, e.g.
echo 1 > /sys/bus/fsl-mc/devices/dprc.1/rescan

Signed-off-by Ioana Ciornei <[email protected]>

---
Changes in v2:
- added proper documentation in /Documentation/ABI/
- updated the MAINTAINERS file
Changes in v3:
- no change

 Documentation/ABI/stable/sysfs-bus-fsl-mc |  6 ++++++
 MAINTAINERS                               |  1 +
 drivers/bus/fsl-mc/dprc-driver.c          |  4 ++--
 drivers/bus/fsl-mc/fsl-mc-bus.c           | 28 ++++++++++++++++++++++++++++
 drivers/bus/fsl-mc/fsl-mc-private.h       |  3 +++
 5 files changed, 40 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/ABI/stable/sysfs-bus-fsl-mc

Documentation/ABI/stable/sysfs-bus-fsl-mc new file +0 -0

new file mode 100644
index 0000000..e530e8c

Quoted 29 lines

Introduce the rescan attribute as a device attribute to synchronize the fsl-mc bus objects and the MC firmware.

To rescan the root dprc only, e.g.
echo 1 > /sys/bus/fsl-mc/devices/dprc.1/rescan

Signed-off-by Ioana Ciornei <[email protected]>

---
Changes in v2:
- added proper documentation in /Documentation/ABI/
- updated the MAINTAINERS file
Changes in v3:
- no change

 Documentation/ABI/stable/sysfs-bus-fsl-mc |  6 ++++++
 MAINTAINERS                               |  1 +
 drivers/bus/fsl-mc/dprc-driver.c          |  4 ++--
 drivers/bus/fsl-mc/fsl-mc-bus.c           | 28 ++++++++++++++++++++++++++++
 drivers/bus/fsl-mc/fsl-mc-private.h       |  3 +++
 5 files changed, 40 insertions(+), 2 deletions(-)  create mode 100644

Documentation/ABI/stable/sysfs-bus-fsl-mc

Documentation/ABI/stable/sysfs-bus-fsl-mc new file +0 -0

new file mode 100644
index 0000000..e530e8c

Documentation/ABI/stable/sysfs-bus-fsl-mc line 2

Quoted 2 lines

Documentation/ABI/stable/sysfs-bus-fsl-mc new file +2 -0

@@ -0,0 +1,6 @@
+What:		/sys/bus/fsl-mc/devices/dprc.*/rescan
+Date:		March. 2018

"."?

Documentation/ABI/stable/sysfs-bus-fsl-mc line 3

Quoted 1 line

Documentation/ABI/stable/sysfs-bus-fsl-mc new file +1 -0

+KernelVersion:	4.16

4.17 :)

Documentation/ABI/stable/sysfs-bus-fsl-mc line 5

Quoted 3 lines

Documentation/ABI/stable/sysfs-bus-fsl-mc new file +2 -0

+Contact:	Ioana Ciornei <[email protected]>
+Description:	Root dprc rescan attribute

What does this do? A bit more description please.

thanks,

greg k-h

Introduce the rescan attribute as a bus attribute to synchronize the fsl-mc bus objects and the MC firmware.

To rescan the fsl-mc bus, e.g.,
echo 1 > /sys/bus/fsl-mc/rescan

Signed-off-by Ioana Ciornei <[email protected]>

---
Changes in v2:
- added proper documentation in /Documentation/ABI/
- updated the MAINTAINERS file
Changes in v3:
- no change

 Documentation/ABI/stable/sysfs-bus-fsl-mc |  7 +++++
 drivers/bus/fsl-mc/fsl-mc-bus.c           | 48 +++++++++++++++++++++++++++++++
 2 files changed, 55 insertions(+)

Documentation/ABI/stable/sysfs-bus-fsl-mc +0 -0

index e530e8c..0663fbd 100644

Documentation/ABI/stable/sysfs-bus-fsl-mc +1 -0

+Users:		Userspace drivers and management tools

drivers/bus/fsl-mc/fsl-mc-bus.c +22 -0

index 9d02984..80010d1 100644
@@ -172,11 +172,59 @@ static ssize_t rescan_store(struct device *dev,
 
 ATTRIBUTE_GROUPS(fsl_mc_dev);
 
+static int scan_fsl_mc_bus(struct device *dev, void *data)
+{
+	struct fsl_mc_device *root_mc_dev;
+	struct fsl_mc_bus *root_mc_bus;
+
+	if (!fsl_mc_is_root_dprc(dev))
+		goto exit;
+
+	root_mc_dev = to_fsl_mc_device(dev);
+	root_mc_bus = to_fsl_mc_bus(root_mc_dev);
+	mutex_lock(&root_mc_bus->scan_mutex);
+	dprc_scan_objects(root_mc_dev, NULL);
+	mutex_unlock(&root_mc_bus->scan_mutex);
+
+exit:
+	return 0;
+}
+
+static ssize_t bus_rescan_store(struct bus_type *bus,
+				const char *buf, size_t count)
+{
+	unsigned long val;

drivers/bus/fsl-mc/fsl-mc-bus.c +4 -0

+
+static struct attribute *fsl_mc_bus_attrs[] = {
+	&bus_attr_rescan.attr,
+	NULL,

Quoted 14 lines

Introduce the rescan attribute as a bus attribute to synchronize the fsl-mc bus objects and the MC firmware.

To rescan the fsl-mc bus, e.g.,
echo 1 > /sys/bus/fsl-mc/rescan

Signed-off-by Ioana Ciornei <[email protected]>

---
Changes in v2:
- added proper documentation in /Documentation/ABI/
- updated the MAINTAINERS file
Changes in v3:
- no change

Quoted 2 lines

 Documentation/ABI/stable/sysfs-bus-fsl-mc |  7 +++++
 drivers/bus/fsl-mc/fsl-mc-bus.c           | 48

+++++++++++++++++++++++++++++++

Quoted 8 lines

 2 files changed, 55 insertions(+)

Documentation/ABI/stable/sysfs-bus-fsl-mc +0 -0

index e530e8c..0663fbd 100644

Documentation/ABI/stable/sysfs-bus-fsl-mc KernelVersion: 4.16 · line 10

Quoted 7 lines

Documentation/ABI/stable/sysfs-bus-fsl-mc +4 -0

@@ -4,3 +4,10 @@ KernelVersion:	4.16
 Contact:	Ioana Ciornei <[email protected]>
 Description:	Root dprc rescan attribute
 Users:		Userspace drivers and management tools
+
+What:		/sys/bus/fsl-mc/rescan
+Date:		March. 2018
+KernelVersion:	4.16

Same comments as previous review.

Documentation/ABI/stable/sysfs-bus-fsl-mc KernelVersion: 4.16 · line 12

Quoted 2 lines

Documentation/ABI/stable/sysfs-bus-fsl-mc +2 -0

+Contact:	Ioana Ciornei <[email protected]>
+Description:	Bus rescan attribute

Again, describe this better please.

Quoted 34 lines

Documentation/ABI/stable/sysfs-bus-fsl-mc +1 -0

+Users:		Userspace drivers and management tools

(quoted diff) +0 -0

b/drivers/bus/fsl-mc/fsl-mc-bus.c index 9d02984..80010d1 100644

drivers/bus/fsl-mc/fsl-mc-bus.c +25 -0

@@ -172,11 +172,59 @@ static ssize_t rescan_store(struct device *dev,
 
 ATTRIBUTE_GROUPS(fsl_mc_dev);
 
+static int scan_fsl_mc_bus(struct device *dev, void *data) {
+	struct fsl_mc_device *root_mc_dev;
+	struct fsl_mc_bus *root_mc_bus;
+
+	if (!fsl_mc_is_root_dprc(dev))
+		goto exit;
+
+	root_mc_dev = to_fsl_mc_device(dev);
+	root_mc_bus = to_fsl_mc_bus(root_mc_dev);
+	mutex_lock(&root_mc_bus->scan_mutex);
+	dprc_scan_objects(root_mc_dev, NULL);
+	mutex_unlock(&root_mc_bus->scan_mutex);
+
+exit:
+	return 0;
+}
+
+static ssize_t bus_rescan_store(struct bus_type *bus,
+				const char *buf, size_t count)
+{
+	unsigned long val;
+
+	if (kstrtoul(buf, 0, &val) < 0)
+		return -EINVAL;
+

drivers/bus/fsl-mc/fsl-mc-bus.c static ssize_t rescan_store(struct device *dev, · line 206

Quoted 6 lines

drivers/bus/fsl-mc/fsl-mc-bus.c +6 -0

+	if (val)
+		bus_for_each_dev(bus, NULL, NULL, scan_fsl_mc_bus);
+
+	return count;
+}
+static BUS_ATTR(rescan, 0220, NULL, bus_rescan_store);

BUS_ATTR_RO()?

Quoted 5 lines

drivers/bus/fsl-mc/fsl-mc-bus.c +4 -0

+
+static struct attribute *fsl_mc_bus_attrs[] = {
+	&bus_attr_rescan.attr,
+	NULL,

drivers/bus/fsl-mc/fsl-mc-bus.c static ssize_t rescan_store(struct device *dev, · line 220

Quoted 10 lines

drivers/bus/fsl-mc/fsl-mc-bus.c +10 -0

+};
+
+static const struct attribute_group fsl_mc_bus_group = {
+	.attrs = fsl_mc_bus_attrs,
+};
+
+static const struct attribute_group *fsl_mc_bus_groups[] = {
+	&fsl_mc_bus_group,
+	NULL,
+};

ATTRIBUTE_GROUPS()?

thanks,

greg k-h

Quoted 74 lines

Quoted 14 lines

Introduce the rescan attribute as a bus attribute to synchronize the fsl-mc bus objects and the MC firmware.

To rescan the fsl-mc bus, e.g.,
echo 1 > /sys/bus/fsl-mc/rescan

Signed-off-by Ioana Ciornei <[email protected]>

---
Changes in v2:
- added proper documentation in /Documentation/ABI/
- updated the MAINTAINERS file
Changes in v3:
- no change

Quoted 2 lines

 Documentation/ABI/stable/sysfs-bus-fsl-mc |  7 +++++
 drivers/bus/fsl-mc/fsl-mc-bus.c           | 48

+++++++++++++++++++++++++++++++

Quoted 8 lines

 2 files changed, 55 insertions(+)

Documentation/ABI/stable/sysfs-bus-fsl-mc +0 -0

index e530e8c..0663fbd 100644

Documentation/ABI/stable/sysfs-bus-fsl-mc KernelVersion: 4.16 · line 10

Quoted 7 lines

Documentation/ABI/stable/sysfs-bus-fsl-mc +4 -0

@@ -4,3 +4,10 @@ KernelVersion:	4.16
 Contact:	Ioana Ciornei <[email protected]>
 Description:	Root dprc rescan attribute
 Users:		Userspace drivers and management tools
+
+What:		/sys/bus/fsl-mc/rescan
+Date:		March. 2018
+KernelVersion:	4.16

Same comments as previous review.

Documentation/ABI/stable/sysfs-bus-fsl-mc KernelVersion: 4.16 · line 12

Quoted 2 lines

Documentation/ABI/stable/sysfs-bus-fsl-mc +2 -0

+Contact:	Ioana Ciornei <[email protected]>
+Description:	Bus rescan attribute

Again, describe this better please.

Quoted 34 lines

Documentation/ABI/stable/sysfs-bus-fsl-mc +1 -0

+Users:		Userspace drivers and management tools

(quoted diff) +0 -0

b/drivers/bus/fsl-mc/fsl-mc-bus.c index 9d02984..80010d1 100644

drivers/bus/fsl-mc/fsl-mc-bus.c +25 -0

@@ -172,11 +172,59 @@ static ssize_t rescan_store(struct device *dev,
 
 ATTRIBUTE_GROUPS(fsl_mc_dev);
 
+static int scan_fsl_mc_bus(struct device *dev, void *data) {
+	struct fsl_mc_device *root_mc_dev;
+	struct fsl_mc_bus *root_mc_bus;
+
+	if (!fsl_mc_is_root_dprc(dev))
+		goto exit;
+
+	root_mc_dev = to_fsl_mc_device(dev);
+	root_mc_bus = to_fsl_mc_bus(root_mc_dev);
+	mutex_lock(&root_mc_bus->scan_mutex);
+	dprc_scan_objects(root_mc_dev, NULL);
+	mutex_unlock(&root_mc_bus->scan_mutex);
+
+exit:
+	return 0;
+}
+
+static ssize_t bus_rescan_store(struct bus_type *bus,
+				const char *buf, size_t count)
+{
+	unsigned long val;
+
+	if (kstrtoul(buf, 0, &val) < 0)
+		return -EINVAL;
+

Quoted 10 lines

drivers/bus/fsl-mc/fsl-mc-bus.c static ssize_t rescan_store(struct device *dev, · line 206

Quoted 6 lines

drivers/bus/fsl-mc/fsl-mc-bus.c +6 -0

+	if (val)
+		bus_for_each_dev(bus, NULL, NULL, scan_fsl_mc_bus);
+
+	return count;
+}
+static BUS_ATTR(rescan, 0220, NULL, bus_rescan_store);

BUS_ATTR_RO()?

Since this is a write-only attribute, a BUS_ATTR_WO would be needed but there is no WO macro defined.