| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <bb2c14f8b2b3de2d69e72013197b11205334e9ca.camel@codeconstruct.com.au>
Date: Tue, 29 Apr 2025 10:58:35 +0800
From: Andrew Jeffery <andrew@...econstruct.com.au>
To: Jean Delvare <jdelvare@...e.de>
Cc: Joel Stanley <joel@....id.au>, Henry Martin <bsdhenrymartin@...il.com>,
Patrick Rudolph <patrick.rudolph@...ements.com>, Andrew Geissler
<geissonator@...oo.com>, Ninad Palsule <ninad@...ux.ibm.com>, Patrick
Venture <venture@...gle.com>, Robert Lippert <roblip@...il.com>,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH 7/7] soc: aspeed: lpc-snoop: Lift channel config to
const structs
Hi Jean,
Thanks for the response and sorry for the delayed reply, I've been on
leave.
On Thu, 2025-04-17 at 12:49 +0200, Jean Delvare wrote:
> Hi Andrew,
>
> On Fri, 11 Apr 2025 10:38:37 +0930, Andrew Jeffery wrote:
> > The shifts and masks for each channel are defined by hardware and
> > are not something that changes at runtime. Accordingly, describe
> > the
> > information in an array of const structs and associate elements
> > with
> > each channel instance, removing the need for the switch and
> > handling of
> > its default case.
>
> I like the idea very much. A few comments on the implementations
> below.
>
> > Signed-off-by: Andrew Jeffery <andrew@...econstruct.com.au>
> > ---
> > drivers/soc/aspeed/aspeed-lpc-snoop.c | 82 +++++++++++++++++------
> > ------------
> > 1 file changed, 41 insertions(+), 41 deletions(-)
> >
> > diff --git a/drivers/soc/aspeed/aspeed-lpc-snoop.c
> > b/drivers/soc/aspeed/aspeed-lpc-snoop.c
> > index
> > 0b2044fd79b1be08dfa33bfcaf249b020c909bb9..b54d8fbf7b83ebadd4fe1b16c
> > bddf07a0bfac868 100644
> > --- a/drivers/soc/aspeed/aspeed-lpc-snoop.c
> > +++ b/drivers/soc/aspeed/aspeed-lpc-snoop.c
> > @@ -10,6 +10,7 @@
> > * 0x80 writes made by the BIOS during the boot process.
> > */
> >
> > +#include "linux/ratelimit.h"
> > #include <linux/bitops.h>
> > #include <linux/clk.h>
> > #include <linux/interrupt.h>
> > @@ -57,7 +58,15 @@ struct aspeed_lpc_snoop_model_data {
> > unsigned int has_hicrb_ensnp;
> > };
> >
> > +struct aspeed_lpc_snoop_channel_cfg {
> > + u32 hicr5_en;
> > + u32 snpwadr_mask;
> > + u32 snpwadr_shift;
> > + u32 hicrb_en;
> > +};
> > +
> > struct aspeed_lpc_snoop_channel {
> > + const struct aspeed_lpc_snoop_channel_cfg *cfg;
> > bool enabled;
> > struct kfifo fifo;
> > wait_queue_head_t wq;
> > @@ -188,7 +197,6 @@ static int aspeed_lpc_enable_snoop(struct
> > aspeed_lpc_snoop *lpc_snoop,
> > int index, u16 lpc_port)
> > {
> > const struct aspeed_lpc_snoop_model_data *model_data;
> > - u32 hicr5_en, snpwadr_mask, snpwadr_shift, hicrb_en;
> > struct aspeed_lpc_snoop_channel *channel;
> > int rc = 0;
> >
> > @@ -200,6 +208,9 @@ static int aspeed_lpc_enable_snoop(struct
> > aspeed_lpc_snoop *lpc_snoop,
> > if (channel->enabled)
> > return -EBUSY;
> >
> > + if (WARN_ONCE(!channel->cfg, "snoop channel %d lacks
> > required config", index))
>
> Why not just WARN? WARN_ONCE has a higher cost, and I don't expect
> this
> code path to be taken more than twice.
Happy to change it.
>
> > + return -EINVAL;
> > +
> > init_waitqueue_head(&channel->wq);
> >
> > channel->miscdev.minor = MISC_DYNAMIC_MINOR;
> > @@ -220,39 +231,20 @@ static int aspeed_lpc_enable_snoop(struct
> > aspeed_lpc_snoop *lpc_snoop,
> > goto err_free_fifo;
> >
> > /* Enable LPC snoop channel at requested port */
> > - switch (index) {
> > - case 0:
> > - hicr5_en = HICR5_EN_SNP0W | HICR5_ENINT_SNP0W;
> > - snpwadr_mask = SNPWADR_CH0_MASK;
> > - snpwadr_shift = SNPWADR_CH0_SHIFT;
> > - hicrb_en = HICRB_ENSNP0D;
> > - break;
> > - case 1:
> > - hicr5_en = HICR5_EN_SNP1W | HICR5_ENINT_SNP1W;
> > - snpwadr_mask = SNPWADR_CH1_MASK;
> > - snpwadr_shift = SNPWADR_CH1_SHIFT;
> > - hicrb_en = HICRB_ENSNP1D;
> > - break;
> > - default:
> > - rc = -EINVAL;
> > - goto err_misc_deregister;
> > - }
> > -
> > - /* Enable LPC snoop channel at requested port */
>
> Strange that you discard a comment which you added yourself in the
> previous patch.
That may have been the result of shuffling the patches while organising
the series. I'll put it back.
>
> > - regmap_update_bits(lpc_snoop->regmap, HICR5, hicr5_en,
> > hicr5_en);
> > - regmap_update_bits(lpc_snoop->regmap, SNPWADR,
> > snpwadr_mask,
> > - lpc_port << snpwadr_shift);
> > + regmap_update_bits(lpc_snoop->regmap, HICR5, channel->cfg-
> > >hicr5_en,
> > + channel->cfg->hicr5_en);
>
> Not caused by your patch, but I think regmap_set_bits() could be used
> here to improve readability.
Ack.
>
> > + regmap_update_bits(lpc_snoop->regmap, SNPWADR, channel-
> > >cfg->snpwadr_mask,
> > + lpc_port << channel->cfg->snpwadr_shift);
> >
> > model_data = of_device_get_match_data(dev);
> > if (model_data && model_data->has_hicrb_ensnp)
> > - regmap_update_bits(lpc_snoop->regmap, HICRB,
> > hicrb_en, hicrb_en);
> > + regmap_update_bits(lpc_snoop->regmap, HICRB,
> > channel->cfg->hicrb_en,
> > + channel->cfg->hicrb_en);
>
> Could also use regmap_set_bits().
Ack.
>
> >
> > channel->enabled = true;
> >
> > return 0;
> >
> > -err_misc_deregister:
> > - misc_deregister(&lpc_snoop->chan[index].miscdev);
> > err_free_fifo:
> > kfifo_free(&lpc_snoop->chan[index].fifo);
> > return rc;
> > @@ -272,21 +264,7 @@ static void aspeed_lpc_disable_snoop(struct
> > aspeed_lpc_snoop *lpc_snoop,
> > if (!channel->enabled)
> > return;
> >
> > - /* Disable interrupts along with the device */
>
> Any reason for killing this poor innocent comment? ^^
None, again, might've been lost in some shuffling.
>
> > - switch (index) {
> > - case 0:
> > - regmap_update_bits(lpc_snoop->regmap, HICR5,
> > - HICR5_EN_SNP0W |
> > HICR5_ENINT_SNP0W,
> > - 0);
> > - break;
> > - case 1:
> > - regmap_update_bits(lpc_snoop->regmap, HICR5,
> > - HICR5_EN_SNP1W |
> > HICR5_ENINT_SNP1W,
> > - 0);
> > - break;
> > - default:
> > - return;
> > - }
> > + regmap_update_bits(lpc_snoop->regmap, HICR5, channel->cfg-
> > >hicr5_en, 0);
>
> Could use regmap_clear_bits() if I'm not mistaken.
Agreed.
>
> >
> > channel->enabled = false;
> > /* Consider improving safety wrt concurrent reader(s) */
> > @@ -294,6 +272,21 @@ static void aspeed_lpc_disable_snoop(struct
> > aspeed_lpc_snoop *lpc_snoop,
> > kfifo_free(&channel->fifo);
> > }
> >
> > +static const struct aspeed_lpc_snoop_channel_cfg channel_cfgs[] =
> > {
> > + {
> > + .hicr5_en = HICR5_EN_SNP0W | HICR5_ENINT_SNP0W,
> > + .snpwadr_mask = SNPWADR_CH0_MASK,
> > + .snpwadr_shift = SNPWADR_CH0_SHIFT,
> > + .hicrb_en = HICRB_ENSNP0D,
> > + },
> > + {
> > + .hicr5_en = HICR5_EN_SNP1W | HICR5_ENINT_SNP1W,
> > + .snpwadr_mask = SNPWADR_CH1_MASK,
> > + .snpwadr_shift = SNPWADR_CH1_SHIFT,
> > + .hicrb_en = HICRB_ENSNP1D,
> > + },
> > +};
> > +
> > static int aspeed_lpc_snoop_probe(struct platform_device *pdev)
> > {
> > struct aspeed_lpc_snoop *lpc_snoop;
> > @@ -308,6 +301,13 @@ static int aspeed_lpc_snoop_probe(struct
> > platform_device *pdev)
> > if (!lpc_snoop)
> > return -ENOMEM;
> >
> > + static_assert(ARRAY_SIZE(channel_cfgs) ==
> > ARRAY_SIZE(lpc_snoop->chan),
> > + "Broken implementation assumption regarding cfg
> > count");
> > + static_assert(ARRAY_SIZE(lpc_snoop->chan) == 2,
> > + "Broken implementation assumption regarding channel
> > count");
>
> Wouldn't it be good (and maybe sufficient) to declare
> aspeed_lpc_snoop_channel_cfg as channel_cfgs[NUM_SNOOP_CHANNELS]?
>
> If you insist on keeping the second assert then you should at least
> use
> NUM_SNOOP_CHANNELS instead of hard-coding 2.
I used the literal 2 here as it reflected the two assignments below...
>
> > + lpc_snoop->chan[0].cfg = &channel_cfgs[0];
> > + lpc_snoop->chan[1].cfg = &channel_cfgs[1];
>
> Could this be done at the beginning of aspeed_lpc_enable_snoop()? So
> that you don't have to duplicate the statement (and don't set
> lpc_snoop->chan[1].cfg if there's no second port). Would save a WARN
> as
> well.
... but implementing your suggestion here will help. Let me rework it.
Thanks,
Andrew
Powered by blists - more mailing lists