| Message ID | 20180504135643.23466-2-antoine.tenart@bootlin.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 05/04/2018 06:56 AM, Antoine Tenart wrote: > The SFF,SFP documentation is clear about making all the DT properties, > with the exception of the compatible, optional. In practice this is not > the case and without an i2c-bus property provided the SFP code will > throw NULL pointer exceptions. > > This patch is an attempt to fix this. > > Signed-off-by: Antoine Tenart <antoine.tenart@bootlin.com> > --- > drivers/net/phy/sfp.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c > index 4ab6e9a50bbe..4686c443fc22 100644 > --- a/drivers/net/phy/sfp.c > +++ b/drivers/net/phy/sfp.c > @@ -298,11 +298,17 @@ static void sfp_set_state(struct sfp *sfp, unsigned int state) > > static int sfp_read(struct sfp *sfp, bool a2, u8 addr, void *buf, size_t len) > { > + if (!sfp->read) > + return -EOPNOTSUPP; -ENODEV would be closer to the intended meaning IMHO, those this could be argue that this is yet another color to paint the bikeshed with. > + > return sfp->read(sfp, a2, addr, buf, len); > } > > static int sfp_write(struct sfp *sfp, bool a2, u8 addr, void *buf, size_t len) > { > + if (!sfp->write) > + return -EOPNOTSUPP; > + > return sfp->write(sfp, a2, addr, buf, len); > } > > @@ -533,6 +539,8 @@ static int sfp_sm_mod_hpower(struct sfp *sfp) > return 0; > > err = sfp_read(sfp, true, SFP_EXT_STATUS, &val, sizeof(val)); > + if (err == -EOPNOTSUPP) > + goto err; > if (err != sizeof(val)) { > dev_err(sfp->dev, "Failed to read EEPROM: %d\n", err); > err = -EAGAIN; > @@ -542,6 +550,8 @@ static int sfp_sm_mod_hpower(struct sfp *sfp) > val |= BIT(0); > > err = sfp_write(sfp, true, SFP_EXT_STATUS, &val, sizeof(val)); > + if (err == -EOPNOTSUPP) > + goto err; > if (err != sizeof(val)) { > dev_err(sfp->dev, "Failed to write EEPROM: %d\n", err); > err = -EAGAIN; > @@ -565,6 +575,8 @@ static int sfp_sm_mod_probe(struct sfp *sfp) > int ret; > > ret = sfp_read(sfp, false, 0, &id, sizeof(id)); > + if (ret == -EOPNOTSUPP) > + return ret; Can you find a way such that only sfp_sm_mod_probe() needs to check whether the sfp read/write operations returned failure and then we just make sure the SFP state machine does not make any more progress? Having to check the sfp_read()/sfp_write() operations all over the place sounds error prone and won't scale in the future.
Hi Florian, On Fri, May 04, 2018 at 10:03:16AM -0700, Florian Fainelli wrote: > On 05/04/2018 06:56 AM, Antoine Tenart wrote: > > > > static int sfp_read(struct sfp *sfp, bool a2, u8 addr, void *buf, size_t len) > > { > > + if (!sfp->read) > > + return -EOPNOTSUPP; > > -ENODEV would be closer to the intended meaning IMHO, those this could > be argue that this is yet another color to paint the bikeshed with. I thought about -ENODEV as well, but ended up choosing -EOPNOTSUPP for some reason. But I'm really fine with both solutions, it really depends on if we want to return a callback isn't available from a s/w point of view (-EOPNOTSUPP) or a h/w point of view (-ENODEV). > > ret = sfp_read(sfp, false, 0, &id, sizeof(id)); > > + if (ret == -EOPNOTSUPP) > > + return ret; > > Can you find a way such that only sfp_sm_mod_probe() needs to check > whether the sfp read/write operations returned failure and then we just > make sure the SFP state machine does not make any more progress? Having > to check the sfp_read()/sfp_write() operations all over the place sounds > error prone and won't scale in the future. I tried doing this in this way (only having logic in the probe function), but that wasn't as simple as this solution and it seemed quite invasive as these read/write calls can be called from a few functions but many code paths (as it's a state machine). So I choose the easiest solution to maintain in the long run, as each future state machine update could impact this. Thanks! Antoine
diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c index 4ab6e9a50bbe..4686c443fc22 100644 --- a/drivers/net/phy/sfp.c +++ b/drivers/net/phy/sfp.c @@ -298,11 +298,17 @@ static void sfp_set_state(struct sfp *sfp, unsigned int state) static int sfp_read(struct sfp *sfp, bool a2, u8 addr, void *buf, size_t len) { + if (!sfp->read) + return -EOPNOTSUPP; + return sfp->read(sfp, a2, addr, buf, len); } static int sfp_write(struct sfp *sfp, bool a2, u8 addr, void *buf, size_t len) { + if (!sfp->write) + return -EOPNOTSUPP; + return sfp->write(sfp, a2, addr, buf, len); } @@ -533,6 +539,8 @@ static int sfp_sm_mod_hpower(struct sfp *sfp) return 0; err = sfp_read(sfp, true, SFP_EXT_STATUS, &val, sizeof(val)); + if (err == -EOPNOTSUPP) + goto err; if (err != sizeof(val)) { dev_err(sfp->dev, "Failed to read EEPROM: %d\n", err); err = -EAGAIN; @@ -542,6 +550,8 @@ static int sfp_sm_mod_hpower(struct sfp *sfp) val |= BIT(0); err = sfp_write(sfp, true, SFP_EXT_STATUS, &val, sizeof(val)); + if (err == -EOPNOTSUPP) + goto err; if (err != sizeof(val)) { dev_err(sfp->dev, "Failed to write EEPROM: %d\n", err); err = -EAGAIN; @@ -565,6 +575,8 @@ static int sfp_sm_mod_probe(struct sfp *sfp) int ret; ret = sfp_read(sfp, false, 0, &id, sizeof(id)); + if (ret == -EOPNOTSUPP) + return ret; if (ret < 0) { dev_err(sfp->dev, "failed to read EEPROM: %d\n", ret); return -EAGAIN;
The SFF,SFP documentation is clear about making all the DT properties, with the exception of the compatible, optional. In practice this is not the case and without an i2c-bus property provided the SFP code will throw NULL pointer exceptions. This patch is an attempt to fix this. Signed-off-by: Antoine Tenart <antoine.tenart@bootlin.com> --- drivers/net/phy/sfp.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)