netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "Pali Rohár" <pali@kernel.org>
To: Andrew Lunn <andrew@lunn.ch>
Cc: "Russell King - ARM Linux admin" <linux@armlinux.org.uk>,
	"David S. Miller" <davem@davemloft.net>,
	"Jakub Kicinski" <kuba@kernel.org>,
	"Thomas Schreiber" <tschreibe@gmail.com>,
	"Heiner Kallweit" <hkallweit1@gmail.com>,
	"Marek Behún" <kabel@kernel.org>,
	netdev@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 1/3] net: sfp: add workaround for Realtek RTL8672 and RTL9601C chips
Date: Thu, 7 Jan 2021 20:17:44 +0100	[thread overview]
Message-ID: <20210107191744.jf76nse35vp6ask3@pali> (raw)
In-Reply-To: <X/dCm1fK9jcjs4XT@lunn.ch>

On Thursday 07 January 2021 18:19:23 Andrew Lunn wrote:
> > +	if (sfp->i2c_block_size < 2) {
> > +		dev_info(sfp->dev, "skipping hwmon device registration "
> > +				   "due to broken EEPROM\n");
> > +		dev_info(sfp->dev, "diagnostic EEPROM area cannot be read "
> > +				   "atomically to guarantee data coherency\n");
> 
> Strings like this are the exception to the 80 character rule. People
> grep for them, and when they are split, they are harder to find.

Ok. I will fix it.

> > -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?

I do not know this part was written by Russel.

Currently function is used in a way if sfp subsystem should switch to
byte IO. So if we are already using byte IO we are not going to do
switch and therefore false is returning.

At least this is how I understood why 'return false' is there.

> >  
> > -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?

Originally I wanted to use just four memcmp() calls but Russel told me
that code should be generic (in case in future initial block size would
be changed, which is a good argument) and come up with this code with
for-loop.

So I think loop is needed.

> 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?

You are right, if code is generic this needs to be fixed to prevent
reading reading undefined memory. I will replace it by proposed min(...)
call.

  parent reply	other threads:[~2021-01-07 19:18 UTC|newest]

Thread overview: 78+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-12-30 15:47 [PATCH 0/4] net: sfp: add support for GPON RTL8672/RTL9601C and Ubiquiti U-Fiber Pali Rohár
2020-12-30 15:47 ` [PATCH 1/4] net: sfp: add workaround for Realtek RTL8672 and RTL9601C chips Pali Rohár
2020-12-30 16:10   ` Russell King - ARM Linux admin
2020-12-30 16:56     ` Pali Rohár
2020-12-30 17:05       ` Russell King - ARM Linux admin
2020-12-30 17:13         ` Andrew Lunn
2020-12-30 17:43           ` Pali Rohár
2020-12-30 19:09             ` Russell King - ARM Linux admin
2020-12-30 19:49               ` Andrew Lunn
2020-12-31 12:14               ` Pali Rohár
2020-12-31 15:09                 ` Andrew Lunn
2020-12-31 15:40                   ` Pali Rohár
2020-12-31 15:30                 ` Andrew Lunn
2020-12-31 17:00                   ` Pali Rohár
2020-12-31 17:13                     ` Andrew Lunn
2021-01-02  1:49                       ` Pali Rohár
2021-01-03  2:25                         ` Thomas Schreiber
2021-01-03  2:41                           ` Pali Rohár
2021-01-06 14:55                             ` Pali Rohár
2021-01-06 15:21                               ` Russell King - ARM Linux admin
2021-01-06 15:23                                 ` Russell King - ARM Linux admin
2021-01-06 15:27                                   ` Russell King - ARM Linux admin
2021-01-06 15:35                                     ` Pali Rohár
2020-12-30 19:30             ` Andrew Lunn
2020-12-30 19:39               ` Russell King - ARM Linux admin
2020-12-30 17:31         ` Pali Rohár
2020-12-30 19:18           ` Russell King - ARM Linux admin
2020-12-30 15:47 ` [PATCH 2/4] net: sfp: allow to use also SFP modules which are detected as SFF Pali Rohár
2020-12-30 16:11   ` Russell King - ARM Linux admin
2020-12-30 17:06     ` Pali Rohár
2020-12-30 17:27       ` Marek Behún
2020-12-30 19:12         ` Russell King - ARM Linux admin
2020-12-31 13:52           ` Pali Rohár
2020-12-30 15:47 ` [PATCH 3/4] net: sfp: assume that LOS is not implemented if both LOS normal and inverted is set Pali Rohár
2020-12-30 16:13   ` Russell King - ARM Linux admin
2020-12-30 16:57     ` Pali Rohár
2020-12-30 17:06       ` Russell King - ARM Linux admin
2020-12-30 17:17         ` Andrew Lunn
2020-12-30 17:32           ` Pali Rohár
2020-12-30 15:47 ` [PATCH 4/4] net: sfp: add mode quirk for GPON module Ubiquiti U-Fiber Instant Pali Rohár
2021-01-06 15:37 ` [PATCH v2 0/3] net: sfp: add support for GPON RTL8672/RTL9601C and Ubiquiti U-Fiber Pali Rohár
2021-01-06 15:37   ` [PATCH v2 1/3] net: sfp: add workaround for Realtek RTL8672 and RTL9601C chips Pali Rohár
2021-01-07  2:02     ` Andrew Lunn
2021-01-07  9:08       ` Pali Rohár
2021-01-07 17:19     ` Andrew Lunn
2021-01-07 17:40       ` Russell King - ARM Linux admin
2021-01-07 19:18         ` Pali Rohár
2021-01-07 19:17       ` Pali Rohár [this message]
2021-01-07 19:45       ` Russell King - ARM Linux admin
2021-01-07 20:21         ` Marek Behún
2021-01-08  0:49           ` Pali Rohár
2021-01-06 15:37   ` [PATCH v2 2/3] net: sfp: assume that LOS is not implemented if both LOS normal and inverted is set Pali Rohár
2021-01-07 16:54     ` Andrew Lunn
2021-01-09 15:46       ` Russell King - ARM Linux admin
2021-01-09 15:54         ` Andrew Lunn
2021-01-09 16:27           ` Russell King - ARM Linux admin
2021-01-09 19:14         ` Pali Rohár
2021-01-09 23:19           ` Russell King - ARM Linux admin
2021-01-09 23:50             ` Pali Rohár
2021-01-06 15:37   ` [PATCH v2 3/3] net: sfp: add mode quirk for GPON module Ubiquiti U-Fiber Instant Pali Rohár
2021-01-07 16:51     ` Andrew Lunn
2021-01-11 11:39 ` [PATCH v3 0/2] net: sfp: add support for GPON RTL8672/RTL9601C and Ubiquiti U-Fiber Pali Rohár
2021-01-11 11:39   ` [PATCH v3 1/2] net: sfp: add workaround for Realtek RTL8672 and RTL9601C chips Pali Rohár
2021-01-11 15:28     ` Marek Behún
2021-01-11 11:39   ` [PATCH v3 2/2] net: sfp: add mode quirk for GPON module Ubiquiti U-Fiber Instant Pali Rohár
2021-01-11 15:32     ` Marek Behún
2021-01-12 13:33   ` [PATCH v3 0/2] net: sfp: add support for GPON RTL8672/RTL9601C and Ubiquiti U-Fiber Pali Rohár
2021-01-18  9:34   ` Pali Rohár
2021-01-25 14:09     ` Pali Rohár
2021-01-25 14:16       ` Russell King - ARM Linux admin
2021-01-25 14:23         ` Pali Rohár
2021-01-25 14:42           ` Russell King - ARM Linux admin
2021-01-25 14:47             ` Pali Rohár
2021-01-25 15:41               ` Andrew Lunn
2021-01-25 15:02 ` [PATCH v4 " Pali Rohár
2021-01-25 15:02   ` [PATCH v4 1/2] net: sfp: add workaround for Realtek RTL8672 and RTL9601C chips Pali Rohár
2021-01-25 15:02   ` [PATCH v4 2/2] net: sfp: add mode quirk for GPON module Ubiquiti U-Fiber Instant Pali Rohár
2021-01-28 21:50   ` [PATCH v4 0/2] net: sfp: add support for GPON RTL8672/RTL9601C and Ubiquiti U-Fiber patchwork-bot+netdevbpf

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210107191744.jf76nse35vp6ask3@pali \
    --to=pali@kernel.org \
    --cc=andrew@lunn.ch \
    --cc=davem@davemloft.net \
    --cc=hkallweit1@gmail.com \
    --cc=kabel@kernel.org \
    --cc=kuba@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux@armlinux.org.uk \
    --cc=netdev@vger.kernel.org \
    --cc=tschreibe@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).