| 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: <20250417124914.77f80975@endymion>
Date: Thu, 17 Apr 2025 12:49:14 +0200
From: Jean Delvare <jdelvare@...e.de>
To: Andrew Jeffery <andrew@...econstruct.com.au>
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 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..b54d8fbf7b83ebadd4fe1b16cbddf07a0bfac868 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.
> + 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.
> - 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.
> + 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().
>
> 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? ^^
> - 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.
>
> 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.
> + 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.
> +
> np = pdev->dev.parent->of_node;
> if (!of_device_is_compatible(np, "aspeed,ast2400-lpc-v2") &&
> !of_device_is_compatible(np, "aspeed,ast2500-lpc-v2") &&
>
--
Jean Delvare
SUSE L3 Support
Powered by blists - more mailing lists