On Tue, Mar 23, 2021 at 12:00AM -0700, Greg KH wrote: > On Tue, Mar 23, 2021 at 11:43:55AM -0700, Don Bollinger wrote: > > On Tue, Mar 23, 2021 at 7:12AM-0700, Greg KH wrote: > > > On Mon, Feb 15, 2021 at 11:38:21AM -0800, Don Bollinger wrote: > > > > optoe is an i2c based driver that supports read/write access to > > > > all the pages (tables) of MSA standard SFP and similar devices > > > > (conforming to the SFF-8472 spec), MSA standard QSFP and similar > > > > devices (conforming to the SFF-8636 spec) and CMIS and similar > > > > devices (conforming to the Common Management Interface > Specfication). > > > > > > > I promise not to engage in a drawn out email exchange over this, but I > > would like to appeal your decision, just once...
Thanks for your response. As promised, I'm done. Is there a correct protocol for withdrawing a patch, or does it just get abandoned? Still trying to be a good citizen. Don > > > > > Given this thread, I think that using the SFP interface/api in the > > > kernel already seems like the best idea forward. > > > > > > That being said, your api here is whack, and I couldn't accept it anyway. > > > > I don't understand. I don't mean you are wrong, I literally don't > > understand what is whack about it. The interface is provided by > > nvmem. I modeled the calls on at24. The layout of the data provided > > by the driver is exactly the same layout that ethtool provides (device, > offset, length). > > Mapping i2c address, page and offset is exactly what ethtool provides. > > So, which part of this is whack? > > It's sysfs. Does nvmem use sysfs for device discovery and enablement? > > nvmem is just a "raw" maping of hardware (memory) to userspace. > > You have a "real" device here that you are trying to also map to userspace, > but when you just expose the "raw" registers (i.e. memory) to userspace, > you are forcing userspace to handle all of the device differences, instead of > the kernel. > > That's fine, for some things, but for anything with a standard, that's not ok, > that's what a kernel is for. > > In other words, you could do what you want today probably with a UIO > driver, just get the kernel out of the way and do it all in userspace. > But that's not a viable or suportable api in the long-run for any standard > hardware type. > > > > Not for the least being it's not even documented in > > > Documentation/ABI/ > > like > > > all sysfs files have to be :) > > > > This could obviously be fixed. I wasn't aware of this directory. Now > > that you've pointed it out, I see that nvmem is actually documented > > there, which is the API I am using. I document that optoe uses the > > nvmem interface, and the mapping of paged memory to linear memory in > > my patch in Documentation/misc-devices/optoe.rst. If you think it > > would be useful, I could provide similar information in > Documentation/ABI/stable. > > Again, nvmem in sysfs is just a dump of the hardware memory. That should > not be how to control a switch device. > > > > And it feels like you are abusing sysfs for things it was not ment > > > for, > > you > > > might want to look into configfs? > > > > I'm using nvmem, which in turn uses sysfs, just like at24. Why should > > optoe be different? I would think it is actually better to use the > > same API (and > > code) as at24, and NOT to put it in a different place. > > at24 too is just an eeprom behind an i2c bus. Accessing it for simple things is > fine for userspace, but not for a standard device type. > > The networking developers have said that they feel the kernel should > properly control devices like this, with a standard api. And I agree with them > (note, I'm biased, I like standard APIs, heck, I've even written specs for > them...) Doing "raw" hardware accesses is great fun for things like one-off > devices (I have Linux running in a keyboard for something like that, also as > my doorbell), but doing this for a "real" > set of devices is not ok. > > Again, it's the difference between the UIO interface and a real ethernet > driver in the kernel. You could just say "all PCI network devices should use > the UIO interface and put the hardware-specific logic in userspace", but > that's not what we (i.e. the Linux kernel developers) feel is the proper way > to handle the abstraction of device types. > > Again, we are kernel developers, we like nice hardware abstractions. > Bonus is that it lets new hardware companies create new devices and no > userspace modifications are needed! I think history is on our side here :) > > > > But really, these are networking devices, so they should be > > > controllable > > using > > > the standard networking apis, not one-off sysfs files. Moving to > > > the > > Linux- > > > standard tools is a good thing, and will work out better in the end > > instead of > > > having to encode lots of device-specific state in userspace like > > > this > > "raw" api > > > seems to require. > > > > This is the real issue. It turns out, on these switches, there are > > two kinds of networking. Linux kernel networking handles one port, of > > 1Gb (or less), which functions as a management port. This is > > typically used for console access. It is configured and managed as an > > ordinary network port, with a kernel network driver and the usual > networking utilities. 'ip addr' > > will show this port as well as loopback ports. The linux kernel has > > no visibility to the switch networking ports. 'ip addr' will not show > > any of the switch networking ports. > > > > The switch functions, switching at 25Tb/s, are completely invisible to > > the linux kernel. The switch ASIC is managed by a device driver > > provided by the ASIC vendor. That driver is driven by management code > > from the ASIC vendor and a host of network applications. Multiple > > vendors compete to provide the best, most innovative, most secure, > > easiest... network capabilities on top of this architecture. NONE of > > them use a kernel network driver, or the layers of control or > > management that the linux kernel offers. On these systems, if you ask > > ethtool to provide EEPROM data, you get 'function not implemented'. > > > > On these systems, SFP/QSFP/CMIS devices are actually not 'networking > > devices' from a Linux kernel perspective. They are GPIO targets and > > EEPROM memory. Switch networking just needs the kernel to toggle the > > GPIO lines and read/write the EEPROM. optoe is just trying to read/write > the EEPROM. > > That sounds like hell. Let's create a proper api for everyone to use, and NOT > provide raw access to random device eeproms (i.e. memory). I thought that > is what switchdev was for. If it is somehow lacking, I'm sure that patches are > gladly accepted. > > Heck, I did a review of the switchdev api and code a long time ago in > response to some companies complaining of just this thing. Sad to see they > never took my advice of "send patches to get your hardware supported in > that api", and persisted in wanting "raw memory" access instead. > > > One last note... The networking folks need a better SFP/QSFP/CMIS > > EEPROM driver to access more pages, and to support the new CMIS > standard. > > Great, work on that! > > But raw eeprom/nvram/ram access is not that api. > > Again, UIO vs. "struct net_device". Think of it that way. > > thanks, > > greg k-h