| Message ID | 20220216160500.2341255-1-alvin@pqrs.dk (mailing list archive) |
|---|---|
| Headers | show |
| Series | net: dsa: realtek: fix PHY register read corruption | expand |
> These two patches fix the issue reported by Arınç where PHY register > reads sometimes return garbage data. > > MAINTAINERS: Please can you help me with the targetting of these two > patches? This bug is present ca. 5.16, when the SMI version of the > rtl8365mb driver was introduced. But now in net-next we have the MDIO > interface from Luiz, where the issue is also present. I am sending what > I think is an ideal patch series, but should I split it up and send the > SMI-related changes to net and the MDIO changes to net-next? If so, how > would I go about splitting it while preventing merge conflicts and build > errors? > > For now I am sending it to net-next so that the whole thing can be > reviewed. If it's applied, I would gladly backport the fix to the stable > tree for 5.16, but I am still confused about what to do for 5.17. > > Thanks for your help. > > > Alvin Šipraga (2): > net: dsa: realtek: allow subdrivers to externally lock regmap > net: dsa: realtek: rtl8365mb: serialize indirect PHY register access > > drivers/net/dsa/realtek/realtek-mdio.c | 46 +++++++++++++++++++++- > drivers/net/dsa/realtek/realtek-smi.c | 48 +++++++++++++++++++++-- > drivers/net/dsa/realtek/realtek.h | 2 + > drivers/net/dsa/realtek/rtl8365mb.c | 54 ++++++++++++++++---------- > 4 files changed, 124 insertions(+), 26 deletions(-) > > -- > 2.35.0 > Thanks for the fix, Alvin. I still feel like we are trying to go around a regmap limitation instead of fixing it there. If we control regmap lock (we can define a custom lock/unlock) and create new regmap_{read,write}_nolock variants, we'll just need to lock the regmap, do whatever you need, and unlock it. BTW, I believe that, for realtek-mdio, a regmap custom lock mechanism could simply use mdio lock while realtek-smi already has priv->lock. Regards,
Luiz Angelo Daros de Luca <luizluca@gmail.com> writes: >> These two patches fix the issue reported by Arınç where PHY register >> reads sometimes return garbage data. >> >> MAINTAINERS: Please can you help me with the targetting of these two >> patches? This bug is present ca. 5.16, when the SMI version of the >> rtl8365mb driver was introduced. But now in net-next we have the MDIO >> interface from Luiz, where the issue is also present. I am sending what >> I think is an ideal patch series, but should I split it up and send the >> SMI-related changes to net and the MDIO changes to net-next? If so, how >> would I go about splitting it while preventing merge conflicts and build >> errors? >> >> For now I am sending it to net-next so that the whole thing can be >> reviewed. If it's applied, I would gladly backport the fix to the stable >> tree for 5.16, but I am still confused about what to do for 5.17. >> >> Thanks for your help. >> >> >> Alvin Šipraga (2): >> net: dsa: realtek: allow subdrivers to externally lock regmap >> net: dsa: realtek: rtl8365mb: serialize indirect PHY register access >> >> drivers/net/dsa/realtek/realtek-mdio.c | 46 +++++++++++++++++++++- >> drivers/net/dsa/realtek/realtek-smi.c | 48 +++++++++++++++++++++-- >> drivers/net/dsa/realtek/realtek.h | 2 + >> drivers/net/dsa/realtek/rtl8365mb.c | 54 ++++++++++++++++---------- >> 4 files changed, 124 insertions(+), 26 deletions(-) >> >> -- >> 2.35.0 >> > > Thanks for the fix, Alvin. > > I still feel like we are trying to go around a regmap limitation > instead of fixing it there. If we control regmap lock (we can define a > custom lock/unlock) and create new regmap_{read,write}_nolock > variants, we'll just need to lock the regmap, do whatever you need, > and unlock it. Can you show me what those regmap_{read,write}_nolock variants would look like in your example? And what about the other regmap_ APIs we use, like regmap_read_poll_timeout, regmap_update_bits, etc. - do you propose to reimplement all of these? > > BTW, I believe that, for realtek-mdio, a regmap custom lock mechanism > could simply use mdio lock while realtek-smi already has priv->lock. Hmm OK. Actually I'm a bit confused about the mdio_lock: can you explain what it's guarding against, for someone unfamiliar with MDIO? Currently realtek-mdio's regmap has an additional lock around it (disable_locking is 0), so with these patches applied the number of locks remains the same. priv->lock is a spinlock which is inappropriate here. I'm not really sure what the point of it is, besides to handle unlocked calls to the _noack function. It might be removable altogether but I would prefer not to touch it for this series. Kind regards, Alvin
> Hmm OK. Actually I'm a bit confused about the mdio_lock: can you explain > what it's guarding against, for someone unfamiliar with MDIO? The more normal use case for MDIO is for PHYs, not switches. There can be multiple PHYs on one MDIO bus. And these PHYs each have there own state machine in phylib. At any point in time, that state machine can request the driver to do something, like poll the PHY status, does it have link? To prevent two PHY drivers trying to use the MDIO bus at the same time, there is an MDIO lock. At the beginning of an MDIO transaction, the lock is taken. And the end of the transaction, reading or writing one register of a device on the bus, the lock is released. So the MDIO lock simply ensures there is only one user of the MDIO bus at one time, for a single read or write. For PHYs this is sufficient. For switches, sometimes you need additional protection. The granularity of an access might not be a single register read or a write. It could be you need to read or write a few registers in an atomic way. If that is the case, you need a lock at a higher level. Andrew
Andrew Lunn <andrew@lunn.ch> writes: >> Hmm OK. Actually I'm a bit confused about the mdio_lock: can you explain >> what it's guarding against, for someone unfamiliar with MDIO? > > The more normal use case for MDIO is for PHYs, not switches. There can > be multiple PHYs on one MDIO bus. And these PHYs each have there own > state machine in phylib. At any point in time, that state machine can > request the driver to do something, like poll the PHY status, does it > have link? To prevent two PHY drivers trying to use the MDIO bus at > the same time, there is an MDIO lock. At the beginning of an MDIO > transaction, the lock is taken. And the end of the transaction, > reading or writing one register of a device on the bus, the lock is > released. > > So the MDIO lock simply ensures there is only one user of the MDIO bus > at one time, for a single read or write. > > For PHYs this is sufficient. For switches, sometimes you need > additional protection. The granularity of an access might not be a > single register read or a write. It could be you need to read or write > a few registers in an atomic way. If that is the case, you need a lock > at a higher level. Thank you Andrew for the clear explanation. Somewhat unrelated to this series, but are you able to explain to me the difference between: mutex_lock(&bus->mdio_lock); and mutex_lock_nested(&bus->mdio_lock, MDIO_MUTEX_NESTED); While looking at other driver examples I noticed the latter form quite a few times too. Kind regards, Alvin
> > I still feel like we are trying to go around a regmap limitation > > instead of fixing it there. If we control regmap lock (we can define a > > custom lock/unlock) and create new regmap_{read,write}_nolock > > variants, we'll just need to lock the regmap, do whatever you need, > > and unlock it. > > Can you show me what those regmap_{read,write}_nolock variants would > look like in your example? And what about the other regmap_ APIs we use, > like regmap_read_poll_timeout, regmap_update_bits, etc. - do you propose > to reimplement all of these? The option of having two regmaps is a nice way to have "_nolock" variants for free. It is much cleaner than any solutions I imagined! Ayway, I don't believe the regmap API expects to have an evil non-locked clone. It looks like it is being abused. What regmap API misses is a way to create a "transaction". Mdio, for example, expects the user to lock the bus before doing a series of accesses while regmap api assumes a single atomic access is enough. However, Realtek indirect register access shows that it is not enough. We could reimplement a mutex for every case where two calls might share the same register (or indirectly affect others like we saw with Realtek) but I believe a shared solution would be better, even if it costs a couple more wrap functions. It would be even nicer if we have a regmap "manual lock" mode that will expose the lock/unlock functions but it will never call them by itself. It would work if it could check if the caller is actually the same thread/context that locked it. However I doubt there is a clean solution in a kernel code that can check if the lock was acquired by the same context that is calling the read. > > BTW, I believe that, for realtek-mdio, a regmap custom lock mechanism > > could simply use mdio lock while realtek-smi already has priv->lock. > > Hmm OK. Actually I'm a bit confused about the mdio_lock: can you explain > what it's guarding against, for someone unfamiliar with MDIO? Currently > realtek-mdio's regmap has an additional lock around it (disable_locking > is 0), so with these patches applied the number of locks remains the > same. Today we already have to redundants locks (mdio and regmap). Your patch is just replacing the regmap lock. regmap_read is something like this: regmap_read lock regmap realtek_mdio_read() lock mdio ... unlock mdio unlock regmap If you are implementing a custom lock, simply use mdio lock directly. And the map_nolock you created does not mean "access without locks" but "you must lock it yourself before using anything here". If that lock is actually mdio_lock, it would be ok to remove the lock inside realtek_mdio_{read,write}. You just need a reference to those lock/unlock functions in realtek_priv. > priv->lock is a spinlock which is inappropriate here. I'm not really > sure what the point of it is, besides to handle unlocked calls to the > _noack function. It might be removable altogether but I would prefer not > to touch it for this series. If spinlock is inappropriate, it can be easily converted to a mutex. Everything else from realtek-mdio might apply. > Kind regards, > Alvin
Luiz Angelo Daros de Luca <luizluca@gmail.com> writes: >> > I still feel like we are trying to go around a regmap limitation >> > instead of fixing it there. If we control regmap lock (we can define a >> > custom lock/unlock) and create new regmap_{read,write}_nolock >> > variants, we'll just need to lock the regmap, do whatever you need, >> > and unlock it. >> >> Can you show me what those regmap_{read,write}_nolock variants would >> look like in your example? And what about the other regmap_ APIs we use, >> like regmap_read_poll_timeout, regmap_update_bits, etc. - do you propose >> to reimplement all of these? > > The option of having two regmaps is a nice way to have "_nolock" > variants for free. It is much cleaner than any solutions I imagined! > Ayway, I don't believe the regmap API expects to have an evil > non-locked clone. It looks like it is being abused. > > What regmap API misses is a way to create a "transaction". Mdio, for > example, expects the user to lock the bus before doing a series of > accesses while regmap api assumes a single atomic access is enough. > However, Realtek indirect register access shows that it is not enough. > We could reimplement a mutex for every case where two calls might > share the same register (or indirectly affect others like we saw with > Realtek) but I believe a shared solution would be better, even if it > costs a couple more wrap functions. > > It would be even nicer if we have a regmap "manual lock" mode that > will expose the lock/unlock functions but it will never call them by > itself. It would work if it could check if the caller is actually the > same thread/context that locked it. However I doubt there is a clean > solution in a kernel code that can check if the lock was acquired by > the same context that is calling the read. I went through all of this while preparing the patch, so your arguments are familiar to me ;-) What I sent was the cleanest solution I could eventually think of. I don't think it is foul play, but I agree it is a bit funny to have this kind of "shadow regmap". However, the interface is quite safe, and as I implied in the commit message, quite foolproof as well. Basically, rather than reimplementing every regmap API that I want to use while manually taking the lock, I just use another regmap with locking disabled. It boils down to exactly the same thing. > > >> > BTW, I believe that, for realtek-mdio, a regmap custom lock mechanism >> > could simply use mdio lock while realtek-smi already has priv->lock. >> >> Hmm OK. Actually I'm a bit confused about the mdio_lock: can you explain >> what it's guarding against, for someone unfamiliar with MDIO? Currently >> realtek-mdio's regmap has an additional lock around it (disable_locking >> is 0), so with these patches applied the number of locks remains the >> same. > > Today we already have to redundants locks (mdio and regmap). Your > patch is just replacing the regmap lock. Is that so? Andrew seems to imply that you shouldn't be using the mdio_lock like this, but only for per-register access, and then implement your own higher level lock: > For PHYs this is sufficient. For switches, sometimes you need > additional protection. The granularity of an access might not be a > single register read or a write. It could be you need to read or write > a few registers in an atomic way. If that is the case, you need a lock > at a higher level. It seems to me like you should have used mdiobus_{read,write} or even mdiobus_{read,write}_nested? Although the state of the art in other DSA drivers seems like a mixed bag, so I don't know. Since I do not have an MDIO switch in front of me to test with, and since the existing MDIO code looks a little suspect, again I would prefer to stick in my lane and just fix the problem without refactoring. > > regmap_read is something like this: > > regmap_read > lock regmap > realtek_mdio_read() > lock mdio > ... > unlock mdio > unlock regmap > > If you are implementing a custom lock, simply use mdio lock directly. > > And the map_nolock you created does not mean "access without locks" > but "you must lock it yourself before using anything here". If that > lock is actually mdio_lock, it would be ok to remove the lock inside > realtek_mdio_{read,write}. You just need a reference to those > lock/unlock functions in realtek_priv. > >> priv->lock is a spinlock which is inappropriate here. I'm not really >> sure what the point of it is, besides to handle unlocked calls to the >> _noack function. It might be removable altogether but I would prefer not >> to touch it for this series. > > If spinlock is inappropriate, it can be easily converted to a mutex. > Everything else from realtek-mdio might apply. Well, this is a bugfix series, not a refactoring. I am not adding more locks than were here before. If I start touching old code (this spinlock predates my engagement with this driver), I will have to answer to that in the commit message too. If we want to do this, let's do it after the bugfix has been reviewed and merged. It will be easier to justify as well. Kind regards, Alvin > >> Kind regards, >> Alvin
> Thank you Andrew for the clear explanation. > > Somewhat unrelated to this series, but are you able to explain to me the > difference between: > > mutex_lock(&bus->mdio_lock); > and > mutex_lock_nested(&bus->mdio_lock, MDIO_MUTEX_NESTED); > > While looking at other driver examples I noticed the latter form quite a > few times too. This is to do with the debug code for checking for deadlocks, CONFIG_PROVE_LOCKING. When that feature is enables, each lock/unlock of a mutex is tracked, and a list is made of what other locks are also taken, and the order. The code can find deadlocks where one thread takes A then B, while another thread takes B and then A. It can also detect when a thread takes lock A and then tries to take lock A again. Rather than track each individual mutex, it uses classes of mutex. So bus->mdio_lock is a class of mutex. The code simply tracks that a bus->mdio_lock has been taken, not a specific bus->mdio_lock. That is generally sufficient, but not always. The mv88e6xxx switch is like many switches, accessed over MDIO. But the mv88e6xxx switch offers an MDIO bus, and there is an MDIO bus driver inside the mv88e6xxx driver. So you have nested MDIO calls. So this debug code seems the same class of mutex being taken twice, and thinks it is a deadlock. You can tell it that nested MDIO calls are actually O.K, it won't deadlock. Andrew
Andrew Lunn <andrew@lunn.ch> writes: >> Thank you Andrew for the clear explanation. >> >> Somewhat unrelated to this series, but are you able to explain to me the >> difference between: >> >> mutex_lock(&bus->mdio_lock); >> and >> mutex_lock_nested(&bus->mdio_lock, MDIO_MUTEX_NESTED); >> >> While looking at other driver examples I noticed the latter form quite a >> few times too. > > This is to do with the debug code for checking for deadlocks, > CONFIG_PROVE_LOCKING. When that feature is enables, each lock/unlock > of a mutex is tracked, and a list is made of what other locks are also > taken, and the order. The code can find deadlocks where one thread > takes A then B, while another thread takes B and then A. It can also > detect when a thread takes lock A and then tries to take lock A again. > > Rather than track each individual mutex, it uses classes of mutex. So > bus->mdio_lock is a class of mutex. The code simply tracks that a > bus->mdio_lock has been taken, not a specific bus->mdio_lock. That is > generally sufficient, but not always. The mv88e6xxx switch is like > many switches, accessed over MDIO. But the mv88e6xxx switch offers an > MDIO bus, and there is an MDIO bus driver inside the mv88e6xxx > driver. So you have nested MDIO calls. So this debug code seems the > same class of mutex being taken twice, and thinks it is a > deadlock. You can tell it that nested MDIO calls are actually O.K, it > won't deadlock. Thanks for the explanation, the missing piece of the puzzle was the fact that some switch drivers expose an additional MDIO bus. I can understand the CONFIG_PROVE_LOCKING rationale. If you have the patience to answer a few more questions: 1. You mentioned in an earlier mail that the mdio_lock is used mostly by PHY drivers to synchronize their access to the MDIO bus, for a single read or write. You also mentioned that for switches which have a more involved access pattern (for instance to access switch management registers), a higher lock is required. In realtek-mdio this is the case: we do a couple of reads and writes over the MDIO bus to access the switch registers. Moreover, the mdio_lock is held for the duration of these MDIO bus reads/writes. Do you mean to say that one should rather take a higher-level lock and only lock/unlock the mdio_lock on a per-read or per-write basis? Put another way, should this: static int realtek_mdio_read(void *ctx, u32 reg, u32 *val) { /* ... */ mutex_lock(&bus->mdio_lock); bus->write(bus, priv->mdio_addr, ...); bus->write(bus, priv->mdio_addr, ...); bus->write(bus, priv->mdio_addr, ...); bus->read(bus, priv->mdio_addr, ...); /* ... */ mutex_unlock(&bus->mdio_lock); return ret; } rather look like this?: static int realtek_mdio_read(void *ctx, u32 reg, u32 *val) { /* ... */ mutex_lock(&my_realtek_driver_lock); /* synchronize concurrent realtek_mdio_{read,write} */ mdiobus_write(bus, priv->mdio_addr, ...); /* mdio_lock locked/unlocked here */ mdiobus_write(bus, priv->mdio_addr, ...); /* ditto */ mdiobus_write(bus, priv->mdio_addr, ...); /* ditto */ mdiobus_read(bus, priv->mdio_addr, ...); /* ditto */ /* ... */ mutex_unlock(&my_realtek_driver_lock); return ret; } 2. Is the nested locking only relevant for DSA switches which offer another MDIO bus? Or should all switch drivers do this, on the basis that, feasibly, one could connect my Realtek switch to the MDIO bus of a mv88e6xxx switch? In that case, and assuming the latter form of raeltek_mdio_read above, should one use the mdiobus_{read,write}_nested functions instead? Kind regards, Alvin
> If you have the patience to answer a few more questions: > > 1. You mentioned in an earlier mail that the mdio_lock is used mostly by > PHY drivers to synchronize their access to the MDIO bus, for a single > read or write. You also mentioned that for switches which have a more > involved access pattern (for instance to access switch management > registers), a higher lock is required. In realtek-mdio this is the case: > we do a couple of reads and writes over the MDIO bus to access the > switch registers. Moreover, the mdio_lock is held for the duration of > these MDIO bus reads/writes. Do you mean to say that one should rather > take a higher-level lock and only lock/unlock the mdio_lock on a > per-read or per-write basis? Put another way, should this: > > static int realtek_mdio_read(void *ctx, u32 reg, u32 *val) > { > /* ... */ > > mutex_lock(&bus->mdio_lock); > > bus->write(bus, priv->mdio_addr, ...); It would be better to use __mdiobus_write() > bus->write(bus, priv->mdio_addr, ...); > bus->write(bus, priv->mdio_addr, ...); > bus->read(bus, priv->mdio_addr, ...); __mdiobus_read() > /* ... */ > > mutex_unlock(&bus->mdio_lock); > > return ret; > } You can do this. > rather look like this?: > > static int realtek_mdio_read(void *ctx, u32 reg, u32 *val) > { > /* ... */ > > mutex_lock(&my_realtek_driver_lock); /* synchronize concurrent realtek_mdio_{read,write} */ > > mdiobus_write(bus, priv->mdio_addr, ...); /* mdio_lock locked/unlocked here */ > mdiobus_write(bus, priv->mdio_addr, ...); /* ditto */ > mdiobus_write(bus, priv->mdio_addr, ...); /* ditto */ > mdiobus_read(bus, priv->mdio_addr, ...); /* ditto */ > > /* ... */ > > mutex_unlock(&my_realtek_driver_lock); > > return ret; > } This would also work. The advantage of this is when you have multiple switches on one MDIO bus, you can allow parallel operations on those switches. Also, if there are PHYs on the MDIO bus as well as the switch, the PHYs can be accessed as well. If you are only doing 3 writes and read, it probably does not matter. If you are going to do a lot of accesses, maybe read all the MIB values, allowing access to the PHYs at the same time would be nice. > 2. Is the nested locking only relevant for DSA switches which offer > another MDIO bus? Or should all switch drivers do this, on the basis > that, feasibly, one could connect my Realtek switch to the MDIO bus of a > mv88e6xxx switch? In that case, and assuming the latter form of > raeltek_mdio_read above, should one use the mdiobus_{read,write}_nested > functions instead? I would suggest you start with plain mdiobus_{read,write}. Using the _nested could potentially hide a deadlock. If somebody does build hardware with this sort of chaining, we can change to the _nested calls. Andrew
From: Alvin Šipraga <alsi@bang-olufsen.dk> These two patches fix the issue reported by Arınç where PHY register reads sometimes return garbage data. MAINTAINERS: Please can you help me with the targetting of these two patches? This bug is present ca. 5.16, when the SMI version of the rtl8365mb driver was introduced. But now in net-next we have the MDIO interface from Luiz, where the issue is also present. I am sending what I think is an ideal patch series, but should I split it up and send the SMI-related changes to net and the MDIO changes to net-next? If so, how would I go about splitting it while preventing merge conflicts and build errors? For now I am sending it to net-next so that the whole thing can be reviewed. If it's applied, I would gladly backport the fix to the stable tree for 5.16, but I am still confused about what to do for 5.17. Thanks for your help. Alvin Šipraga (2): net: dsa: realtek: allow subdrivers to externally lock regmap net: dsa: realtek: rtl8365mb: serialize indirect PHY register access drivers/net/dsa/realtek/realtek-mdio.c | 46 +++++++++++++++++++++- drivers/net/dsa/realtek/realtek-smi.c | 48 +++++++++++++++++++++-- drivers/net/dsa/realtek/realtek.h | 2 + drivers/net/dsa/realtek/rtl8365mb.c | 54 ++++++++++++++++---------- 4 files changed, 124 insertions(+), 26 deletions(-)