Re: [PATCH] optoe: driver to read/write SFP/QSFP EEPROMs

[Arnd Bergmann]

On Thu, Jun 14, 2018 at 2:40 AM, Don Bollinger <don@xxxxxxxxxxxxxxxxx> wrote:
> On Mon, Jun 11, 2018 at 03:43:02PM +0200, Arnd Bergmann wrote:
>> On Mon, Jun 11, 2018 at 6:25 AM, Don Bollinger <don@xxxxxxxxxxxxxxxxx> wrote:

>>
>> I don't understand this part: I see some older patches introducing an
>> EEPROM_CLASS, but nothing ever seems to have made it into the
>> mainline kernel.
>>
>> If that class isn't there, this code shouldn't be either. You can always
>> add it back in case we decide to introduce that class later, but then
>> I wouldn't make it a compile-time option but just a hard dependency
>> instead.
>
> Thanks for the feedback.
>
> Some background will explain how optoe got here...  My goal is to make
> the EEPROM data and controls in {Q}SFP devices more accessible.  (I'm
> working for Finisar, an optics vendor.)
>
> The ecosystem where optoe operates includes switch vendors, NOS vendors,
> optics vendors and switch ASIC vendors, competing and collaborating to
> build a bunch of different complete white box switch solutions.
>
> The NOS (Network Operating System) vendors start with a Linux distro,
> then add a bunch of their own patches and 'value add'.  Then they
> include platform drivers from the switch vendors.  And they integrate
> the chosen switch ASIC SDK.
>
> Here's the key:  I don't have access to all of those pieces.  I can
> build an environment to build my driver for many of those systems, but I
> can't change other elements of that NOS.
>
> The first NOS I targeted (Cumulus Linux) happens to be the author of the
> EEPROM_CLASS patches you cited.  I began with their driver (the best
> available), and morphed it into optoe.  To fit their environment, I kept
> the EEPROM_CLASS code.  They use their own platform driver (one for each
> switch model) to instantiate their predecessor to the optoe driver.  So,
> since I don't have access to their platform driver, I kept the data
> structure they pass to the probe function, where I get initialization
> data.  That is now struct optoe_platform_data.  The *dummy items in that
> struct maintain compatibility, while making it clear that they aren't
> really needed.
>
> When I targeted the next NOS, it did not have the EEPROM_CLASS code.  I
> figured out I could #ifdef that code, enabling me to create a driver that
> works in both environments.

Ok, I see. For the upstream submission of course, none of the forked
kernel code bases matter at all, what we want is a driver that makes
sense by itself, and none of it should depend on any third party code.

>> > +struct optoe_platform_data {
>> > +       u32             byte_len;               /* size (sum of all addr) */
>> > +       u16             page_size;              /* for writes */
>> > +       u8              flags;
>> > +       void            *dummy1;                /* backward compatibility */
>> > +       void            *dummy2;                /* backward compatibility */
>> > +
>> > +#ifdef EEPROM_CLASS
>> > +       struct eeprom_platform_data *eeprom_data;
>> > +#endif
>> > +       char port_name[MAX_PORT_NAME_LEN];
>> > +};
>>
>> What is the backward-compatibility for? Normally we don't do it
>> like that, we only keep compatibility with things that have already
>> been part of the kernel, especially when you also have an #ifdef
>> that makes it incompatible again ;-)
>
> So it is actually compatible with two different kernel compilations.  If
> EEPROM_CLASS is configured, optoe supports that model.  If not, optoe
> implements a sysfs file to identify which port this device is on.  It
> works.

I mean the 'dummy1' and 'dummy2' variables. They don't seem to make
any sense, and neither does the comment here.

>> > +struct optoe_data {
>> > +       struct optoe_platform_data chip;
>>
>> Maybe merge the two structures into one?
>
> optoe_platform_data is passed in when optoe gets a probe.  It is only
> part of the data I need to maintain internally.  I inherited this
> pattern from at24.c (the predecessor ^2 to optoe).

Passed in from where? The structure is declared locally in this
file, so no other driver has access to the definition.

For traditional devices, we would use a header in
include/linux/platform_data/, but a more modern way of doing this
would be to use named device properties that are either put
in the devicetree file (on embedded machines) or added through
the .properties field when statically declaring an i2c device from
a PCI device parent.

      Arnd