Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Wed, 13 Jun 2018 17:40:44 -0700
From:   Don Bollinger <don@thebollingers.org>
To:     Arnd Bergmann <arnd@arndb.de>
Cc:     Arnd Bergmann <arnd@arndb.de>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] optoe: driver to read/write SFP/QSFP EEPROMs
Message-ID: <20180614004044.jjz3ws3zyns7x4x6@thebollingers.org>
References: <20180611042515.ml6zbcmz6dlvjmrp@thebollingers.org>
 <CAK8P3a2aPshgrAp6=e62mvEpmmLYCAwRa_HxG_A1YKJ5we5XVQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAK8P3a2aPshgrAp6=e62mvEpmmLYCAwRa_HxG_A1YKJ5we5XVQ@mail.gmail.com>
User-Agent: NeoMutt/20170113 (1.7.2)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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@thebollingers.org> 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) and MSA standard QSFP and similar devices
> > (conforming to the SFF-8436 spec).
> >
> >
> > Signed-off-by: Don Bollinger <don@thebollingers.org>
> 
> > +
> > +#undef EEPROM_CLASS
> > +#ifdef CONFIG_EEPROM_CLASS
> > +#define EEPROM_CLASS
> > +#endif
> > +#ifdef CONFIG_EEPROM_CLASS_MODULE
> > +#define EEPROM_CLASS
> > +#endif
> 
> 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.  

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

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

> 
>         Arnd
> 

All that said...  I am working with my partners to see if we can remove
both the EEPROM_CLASS code and the compatibility anomalies.  Technically
it would be easy.  Logistically it is probably manageable.  The next
step is mine, I will either remove the offending code or I will lobby to
keep it in there.

Don