[Intel-wired-lan] [RFC PATCH 0/4] r8169: support dash

[Intel-wired-lan] [RFC PATCH 0/4] r8169: support dash

[Jakub Kicinski]

On Mon, 29 Nov 2021 18:13:11 +0800 Hayes Wang wrote:
> These patches are used to support dash for RTL8111EP and
> RTL8111FP(RTL81117).

If I understand correctly DASH is a DMTF standard for remote control.

Since it's a standard I think we should have a common way of
configuring it across drivers. Is enable/disable the only configuration
that we will need?

We don't use sysfs too much these days, can we move the knob to
devlink, please? (If we only need an on/off switch generic devlink param
should be fine).

[Intel-wired-lan] [RFC PATCH 0/4] r8169: support dash

[Hayes Wang]

Jakub Kicinski <kuba@kernel.org>
> Sent: Tuesday, November 30, 2021 2:00 AM
> Subject: Re: [RFC PATCH 0/4] r8169: support dash
> 
> On Mon, 29 Nov 2021 18:13:11 +0800 Hayes Wang wrote:
> > These patches are used to support dash for RTL8111EP and
> > RTL8111FP(RTL81117).
> 
> If I understand correctly DASH is a DMTF standard for remote control.
> 
> Since it's a standard I think we should have a common way of
> configuring it across drivers.

Excuse me. I am not familiar with it.
What document or sample code could I start?

> Is enable/disable the only configuration
> that we will need?

I don't think I could answer it before I understand the above way
you mentioned.

> We don't use sysfs too much these days, can we move the knob to
> devlink, please? (If we only need an on/off switch generic devlink param
> should be fine).

Thanks. I would study devlink.

Best Regards,
Hayes

[Intel-wired-lan] [RFC PATCH 0/4] r8169: support dash

[Jakub Kicinski]

On Wed, 1 Dec 2021 02:57:00 +0000 Hayes Wang wrote:
> Jakub Kicinski <kuba@kernel.org>
> > Sent: Tuesday, November 30, 2021 2:00 AM
> > Subject: Re: [RFC PATCH 0/4] r8169: support dash
> > 
> > On Mon, 29 Nov 2021 18:13:11 +0800 Hayes Wang wrote:  
> > > These patches are used to support dash for RTL8111EP and
> > > RTL8111FP(RTL81117).  
> > 
> > If I understand correctly DASH is a DMTF standard for remote control.
> > 
> > Since it's a standard I think we should have a common way of
> > configuring it across drivers.  
> 
> Excuse me. I am not familiar with it.
> What document or sample code could I start?
> 
> > Is enable/disable the only configuration
> > that we will need?  
> 
> I don't think I could answer it before I understand the above way
> you mentioned.
> 
> > We don't use sysfs too much these days, can we move the knob to
> > devlink, please? (If we only need an on/off switch generic devlink param
> > should be fine).  
> 
> Thanks. I would study devlink.

I'm not sure how relevant it will be to you but this is the
documentation we have:

https://www.kernel.org/doc/html/latest/networking/devlink/index.html
https://www.kernel.org/doc/html/latest/networking/devlink/devlink-params.html

You'll need to add a generic parameter (define + a short description)
like 325e0d0aa683 ("devlink: Add 'enable_iwarp' generic device param")

In terms of driver changes I think the most relevant example to you
will be:

drivers/net/ethernet/ti/cpsw_new.c

You need to call devlink_alloc(), devlink_register and
devlink_params_register() (and the inverse functions).

[Intel-wired-lan] [RFC PATCH 0/4] r8169: support dash

[Hayes Wang]

Jakub Kicinski <kuba@kernel.org>
> Sent: Wednesday, December 1, 2021 11:09 AM
[...]
> I'm not sure how relevant it will be to you but this is the
> documentation we have:
> 
> https://www.kernel.org/doc/html/latest/networking/devlink/index.html
> https://www.kernel.org/doc/html/latest/networking/devlink/devlink-params.ht
> ml
> 
> You'll need to add a generic parameter (define + a short description)
> like 325e0d0aa683 ("devlink: Add 'enable_iwarp' generic device param")
> 
> In terms of driver changes I think the most relevant example to you
> will be:
> 
> drivers/net/ethernet/ti/cpsw_new.c
> 
> You need to call devlink_alloc(), devlink_register and
> devlink_params_register() (and the inverse functions).

I have studied the devlink briefly.

However, I find some problems. First, our
settings are dependent on the design of
both the hardware and firmware. That is,
I don't think the others need to do the
settings as the same as us. The devlink
seems to let everyone could use the same
command to do the same setting. However,
most of our settings are useless for the
other devices.

Second, according to the design of our
CMAC, the application has to read and
write data with variable length from/to
the firmware. Each custom has his own
requests. Therefore, our customs would
get different firmware with different
behavior. Only the application and the
firmware know how to communicate with
each other. The driver only passes the
data between them. Like the Ethernet
driver, it doesn't need to know the
contend of the packet. I could implement
the CMAC through sysfs, but I don't
know how to do by devlink.

In brief, CMAC is our major method to
configure the firmware and get response
from the firmware. Except for certain information,
the other settings are not standard and useless
for the other vendors.

Is the devlink the only method I could use?
Actually, we use IOCTL now. We wish to
convert it to sysfs for upstream driver.

Best Regards,
Hayes

[Intel-wired-lan] [RFC PATCH 0/4] r8169: support dash

[Jakub Kicinski]

On Fri, 3 Dec 2021 07:57:08 +0000 Hayes Wang wrote:
> Jakub Kicinski <kuba@kernel.org>
> > I'm not sure how relevant it will be to you but this is the
> > documentation we have:
> > 
> > https://www.kernel.org/doc/html/latest/networking/devlink/index.html
> > https://www.kernel.org/doc/html/latest/networking/devlink/devlink-params.ht
> > ml
> > 
> > You'll need to add a generic parameter (define + a short description)
> > like 325e0d0aa683 ("devlink: Add 'enable_iwarp' generic device param")
> > 
> > In terms of driver changes I think the most relevant example to you
> > will be:
> > 
> > drivers/net/ethernet/ti/cpsw_new.c
> > 
> > You need to call devlink_alloc(), devlink_register and
> > devlink_params_register() (and the inverse functions).  
> 
> I have studied the devlink briefly.
> 
> However, I find some problems. First, our
> settings are dependent on the design of
> both the hardware and firmware. That is,
> I don't think the others need to do the
> settings as the same as us. The devlink
> seems to let everyone could use the same
> command to do the same setting. However,
> most of our settings are useless for the
> other devices.
> 
> Second, according to the design of our
> CMAC, the application has to read and
> write data with variable length from/to
> the firmware. Each custom has his own
> requests. Therefore, our customs would
> get different firmware with different
> behavior. Only the application and the
> firmware know how to communicate with
> each other. The driver only passes the
> data between them. Like the Ethernet
> driver, it doesn't need to know the
> contend of the packet. I could implement
> the CMAC through sysfs, but I don't
> know how to do by devlink.
> 
> In brief, CMAC is our major method to
> configure the firmware and get response
> from the firmware. Except for certain information,
> the other settings are not standard and useless
> for the other vendors.
> 
> Is the devlink the only method I could use?
> Actually, we use IOCTL now. We wish to
> convert it to sysfs for upstream driver.

Ah, I've only spotted the enable/disable knob in the patch. 
If you're exchanging arbitrary binary data with the FW we 
can't help you. It's not going to fly upstream.

[Intel-wired-lan] [RFC PATCH 0/4] r8169: support dash

[Alexander Lobakin]

From: Jakub Kicinski <kuba@kernel.org>
Date: Fri, 3 Dec 2021 07:04:10 -0800

> On Fri, 3 Dec 2021 07:57:08 +0000 Hayes Wang wrote:
> > Jakub Kicinski <kuba@kernel.org>
> > > I'm not sure how relevant it will be to you but this is the
> > > documentation we have:
> > > 
> > > https://www.kernel.org/doc/html/latest/networking/devlink/index.html
> > > https://www.kernel.org/doc/html/latest/networking/devlink/devlink-params.ht
> > > ml
> > > 
> > > You'll need to add a generic parameter (define + a short description)
> > > like 325e0d0aa683 ("devlink: Add 'enable_iwarp' generic device param")
> > > 
> > > In terms of driver changes I think the most relevant example to you
> > > will be:
> > > 
> > > drivers/net/ethernet/ti/cpsw_new.c
> > > 
> > > You need to call devlink_alloc(), devlink_register and
> > > devlink_params_register() (and the inverse functions).  
> > 
> > I have studied the devlink briefly.
> > 
> > However, I find some problems. First, our
> > settings are dependent on the design of
> > both the hardware and firmware. That is,
> > I don't think the others need to do the
> > settings as the same as us. The devlink
> > seems to let everyone could use the same
> > command to do the same setting. However,
> > most of our settings are useless for the
> > other devices.
> > 
> > Second, according to the design of our
> > CMAC, the application has to read and
> > write data with variable length from/to
> > the firmware. Each custom has his own
> > requests. Therefore, our customs would
> > get different firmware with different
> > behavior. Only the application and the
> > firmware know how to communicate with
> > each other. The driver only passes the
> > data between them. Like the Ethernet
> > driver, it doesn't need to know the
> > contend of the packet. I could implement
> > the CMAC through sysfs, but I don't
> > know how to do by devlink.
> > 
> > In brief, CMAC is our major method to
> > configure the firmware and get response
> > from the firmware. Except for certain information,
> > the other settings are not standard and useless
> > for the other vendors.
> > 
> > Is the devlink the only method I could use?
> > Actually, we use IOCTL now. We wish to
> > convert it to sysfs for upstream driver.
> 
> Ah, I've only spotted the enable/disable knob in the patch. 
> If you're exchanging arbitrary binary data with the FW we 
> can't help you. It's not going to fly upstream.

Uhm. I'm not saying sysfs is a proper way to do that, not at all,
buuut...
We have a ton of different subsystems providing a communication
channel between userspace and HW/FW. Chardevices all over the
tree, highly used rpmsg for remoteproc, uio. We have register
dump in Ethtool, as well as get/set for EEPROM, I'd count them
as well.
So it probably isn't a bad idea to provide some standard API for
network drivers to talk to HW/FW from userspace, like get/set or
rx/tx (when having enough caps for sure)? It could be Devlink ops
or Ethtool ops, the latter fits more to me.

Al

[Intel-wired-lan] [RFC PATCH 0/4] r8169: support dash

[Jakub Kicinski]

On Sat,  4 Dec 2021 02:08:29 +0100 Alexander Lobakin wrote:
> > Ah, I've only spotted the enable/disable knob in the patch. 
> > If you're exchanging arbitrary binary data with the FW we 
> > can't help you. It's not going to fly upstream.  
> 
> Uhm. I'm not saying sysfs is a proper way to do that, not at all,
> buuut...
> We have a ton of different subsystems providing a communication
> channel between userspace and HW/FW. Chardevices all over the
> tree, highly used rpmsg for remoteproc, uio. 

Not in Ethernet.

> We have register dump in Ethtool,

Read only.

> as well as get/set for EEPROM, I'd count them as well.

EEPROM writes are supposed to update FW images, not send random
messages.

> So it probably isn't a bad idea to provide some standard API for
> network drivers to talk to HW/FW from userspace, like get/set or
> rx/tx (when having enough caps for sure)? It could be Devlink ops
> or Ethtool ops, the latter fits more to me.

I'm not saying it's wrong to merge shim drivers into the kernel and
let the user space talk to device FW. I'm saying it's counter to what
netdev's policy has always been and counter to my personal interests.

What is a standard API for custom, proprietary FW message interface?
We want standards at a functional level. Once you open up a raw FW
write interface there is no policing of what goes thru it.

I CCed Intel since you also have the (infamous) ME, but I never heard
of the need to communicate from the OS to the ME via the netdev
driver... Not sure why things are different for Realtek.

[Intel-wired-lan] [RFC PATCH 0/4] r8169: support dash

[Hayes Wang]

Jakub Kicinski <kuba@kernel.org>
> Sent: Friday, December 3, 2021 11:04 PM
[...]
> Ah, I've only spotted the enable/disable knob in the patch.
> If you're exchanging arbitrary binary data with the FW we
> can't help you. It's not going to fly upstream. 

How is it that we only provide certain basic settings,
such as IPv4 address, IPv6 address, and so on? Then,
they are not the arbitrary binary data.

Could devlink param be used for more than 4 bytes settings?
At least the IPv6 address is longer.

Besides, we need the information of SMBIOS which could
be 4K ~ 8K bytes data. Is there any way we could transmit
it to firmware?

Best Regards,
Hayes

[Intel-wired-lan] [RFC PATCH 0/4] r8169: support dash

[Jakub Kicinski]

On Tue, 7 Dec 2021 07:28:02 +0000 Hayes Wang wrote:
> Jakub Kicinski <kuba@kernel.org>
> > Ah, I've only spotted the enable/disable knob in the patch.
> > If you're exchanging arbitrary binary data with the FW we
> > can't help you. It's not going to fly upstream.   
> 
> How is it that we only provide certain basic settings,
> such as IPv4 address, IPv6 address, and so on? Then,
> they are not the arbitrary binary data.
> 
> Could devlink param be used for more than 4 bytes settings?
> At least the IPv6 address is longer.

We can add a new devlink sub-command and driver callback in that case.

> Besides, we need the information of SMBIOS which could
> be 4K ~ 8K bytes data. Is there any way we could transmit
> it to firmware?

Is structure of that data defined by some DMTF standard?

[Intel-wired-lan] [RFC PATCH 0/4] r8169: support dash

[Hayes Wang]

Jakub Kicinski <kuba@kernel.org>
> Sent: Wednesday, December 8, 2021 12:21 PM
[...]
> > Could devlink param be used for more than 4 bytes settings?
> > At least the IPv6 address is longer.
> 
> We can add a new devlink sub-command and driver callback in that case.

Excuse me. Do you mean someone will add it? Then, I could use it.
Or, I have to add it.

> > Besides, we need the information of SMBIOS which could
> > be 4K ~ 8K bytes data. Is there any way we could transmit
> > it to firmware?
> 
> Is structure of that data defined by some DMTF standard?
Yes.

Best Regards,
Hayes

[Intel-wired-lan] [RFC PATCH 0/4] r8169: support dash

[Jakub Kicinski]

On Wed, 8 Dec 2021 07:53:28 +0000 Hayes Wang wrote:
> Jakub Kicinski <kuba@kernel.org>
> > Sent: Wednesday, December 8, 2021 12:21 PM  
> > > Could devlink param be used for more than 4 bytes settings?
> > > At least the IPv6 address is longer.  
> > 
> > We can add a new devlink sub-command and driver callback in that case.  
> 
> Excuse me. Do you mean someone will add it? Then, I could use it.
> Or, I have to add it.

You'd need to write all the code.

> > > Besides, we need the information of SMBIOS which could
> > > be 4K ~ 8K bytes data. Is there any way we could transmit
> > > it to firmware?  
> > 
> > Is structure of that data defined by some DMTF standard?  
> Yes.

That's good, as long as the kernel parses and validates the messages
the exact interface to user space does not matter that much.

[Intel-wired-lan] [RFC PATCH 0/4] r8169: support dash

[Hayes Wang]

Jakub Kicinski <kuba@kernel.org>
> Sent: Thursday, December 9, 2021 5:38 AM
[...]
> > Excuse me. Do you mean someone will add it? Then, I could use it.
> > Or, I have to add it.
> 
> You'd need to write all the code.
I need more time to think how to do.
Thanks.

Best Regards,
Hayes