| Message ID | 20211129101315.16372-381-nic_swsd@realtek.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | r8169: support dash | expand |
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).
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
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).
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
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.
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
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.
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
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?
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
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.
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