On Thu, Jan 07, 2021 at 06:19:23PM +0100, Andrew Lunn wrote:
> > -static int sfp_quirk_i2c_block_size(const struct sfp_eeprom_base *base)
> > +static bool sfp_id_needs_byte_io(struct sfp *sfp, void *buf, size_t len)
> > {
> > - if (!memcmp(base->vendor_name, "VSOL ", 16))
> > - return 1;
> > - if (!memcmp(base->vendor_name, "OEM ", 16) &&
> > - !memcmp(base->vendor_pn, "V2801F ", 16))
> > - return 1;
> > + size_t i, block_size = sfp->i2c_block_size;
> >
> > - /* Some modules can't cope with long reads */
> > - return 16;
> > -}
> > + /* Already using byte IO */
> > + if (block_size == 1)
> > + return false;
>
> This seems counter intuitive. We don't need byte IO because we are
> doing btye IO? Can we return True here?
It is counter-intuitive, but as this is indicating whether we need to
switch to byte IO, if we're already doing byte IO, then we don't need
to switch.
> > -static void sfp_quirks_base(struct sfp *sfp, const struct sfp_eeprom_base
> > *base)
> > -{
> > - sfp->i2c_block_size = sfp_quirk_i2c_block_size(base);
> > + for (i = 1; i < len; i += block_size) {
> > + if (memchr_inv(buf + i, '\0', block_size - 1))
> > + return false;
> > + }
>
> Is the loop needed?
I think you're not reading the code very well. It checks for bytes at
offset 1..blocksize-1, blocksize+1..2*blocksize-1, etc are zero. It
does _not_ check that byte 0 or the byte at N*blocksize is zero - these
bytes are skipped. In other words, the first byte of each transfer can
be any value. The other bytes of the _entire_ ID must be zero.
> I also wonder if on the last iteration of the loop you go passed the
> end of buf? Don't you need a min(block_size -1, len - i) or
> similar?
The ID is 64 bytes long, and is fixed. block_size could be a non-power
of two, but that is highly unlikely. block_size will never be larger
than 16 either.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
