Messages in this thread | | | Date | Mon, 21 Feb 2022 21:06:19 +0200 | From | Vladimir Oltean <> | Subject | Re: [PATCH v2 net-next 1/2] net: dsa: realtek: allow subdrivers to externally lock regmap |
| |
On Mon, Feb 21, 2022 at 07:46:30PM +0100, Alvin Šipraga wrote: > From: Alvin Šipraga <alsi@bang-olufsen.dk> > > Currently there is no way for Realtek DSA subdrivers to serialize > consecutive regmap accesses. In preparation for a bugfix relating to > indirect PHY register access - which involves a series of regmap > reads and writes - add a facility for subdrivers to serialize their > regmap access. > > Specifically, a mutex is added to the driver private data structure and > the standard regmap is initialized with custom lock/unlock ops which use > this mutex. Then, a "nolock" variant of the regmap is added, which is > functionally equivalent to the existing regmap except that regmap > locking is disabled. Functions that wish to serialize a sequence of > regmap accesses may then lock the newly introduced driver-owned mutex > before using the nolock regmap. > > Doing things this way means that subdriver code that doesn't care about > serialized register access - i.e. the vast majority of code - needn't > worry about synchronizing register access with an external lock: it can > just continue to use the original regmap. > > Another advantage of this design is that, while regmaps with locking > disabled do not expose a debugfs interface for obvious reasons, there > still exists the original regmap which does expose this interface. This > interface remains safe to use even combined with driver codepaths that > use the nolock regmap, because said codepaths will use the same mutex > to synchronize access. > > With respect to disadvantages, it can be argued that having > near-duplicate regmaps is confusing. However, the naming is rather > explicit, and examples will abound. > > Finally, while we are at it, rename realtek_smi_mdio_regmap_config to > realtek_smi_regmap_config. This makes it consistent with the naming > realtek_mdio_regmap_config in realtek-mdio.c. > > Signed-off-by: Alvin Šipraga <alsi@bang-olufsen.dk> > ---
Reviewed-by: Vladimir Oltean <olteanv@gmail.com>
> drivers/net/dsa/realtek/realtek-mdio.c | 46 ++++++++++++++++++++++-- > drivers/net/dsa/realtek/realtek-smi.c | 48 ++++++++++++++++++++++++-- > drivers/net/dsa/realtek/realtek.h | 2 ++ > 3 files changed, 91 insertions(+), 5 deletions(-) > > diff --git a/drivers/net/dsa/realtek/realtek-mdio.c b/drivers/net/dsa/realtek/realtek-mdio.c > index 0308be95d00a..31e1f100e48e 100644 > --- a/drivers/net/dsa/realtek/realtek-mdio.c > +++ b/drivers/net/dsa/realtek/realtek-mdio.c > @@ -98,6 +98,20 @@ static int realtek_mdio_read(void *ctx, u32 reg, u32 *val) > return ret; > } > > +static void realtek_mdio_lock(void *ctx)
I looked even at the build results for v1 and I don't know exactly why sparse doesn't complain about the lack of __acquires() and __releases() annotations, to avoid context imbalance warnings.
https://patchwork.kernel.org/project/netdevbpf/patch/20220216160500.2341255-2-alvin@pqrs.dk/
Anyway, those aren't of much use IMO (you can't get it to do anything but very trivial checks), and if sparse doesn't complain for whatever reason, I certainly won't request you to add them.
I see other implementations don't have them either - like vexpress_config_lock.
| |