1. Patchew
  2. linux
  3. [PATCH v2 00/10] soc: aspeed: lpc-snoop: Miscellaneous fixes
  4. View patch

[PATCH v2 04/10] soc: aspeed: lpc-snoop: Constrain parameters in channel paths

Andrew Jeffery posted 10 patches 3 months, 3 weeks ago
[PATCH v2 04/10] soc: aspeed: lpc-snoop: Constrain parameters in channel paths
Posted by Andrew Jeffery 3 months, 3 weeks ago 
Ensure pointers and the channel index are valid before use.

Signed-off-by: Andrew Jeffery <andrew@codeconstruct.com.au>
---
 drivers/soc/aspeed/aspeed-lpc-snoop.c | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

diff --git a/drivers/soc/aspeed/aspeed-lpc-snoop.c b/drivers/soc/aspeed/aspeed-lpc-snoop.c
index ca7536213e0986f737606a52996ffea620df2a7a..804c6ed9c4c671da73a6c66c1de41c59922c82dc 100644
--- a/drivers/soc/aspeed/aspeed-lpc-snoop.c
+++ b/drivers/soc/aspeed/aspeed-lpc-snoop.c
@@ -25,7 +25,6 @@
 
 #define DEVICE_NAME	"aspeed-lpc-snoop"
 
-#define NUM_SNOOP_CHANNELS 2
 #define SNOOP_FIFO_SIZE 2048
 
 #define HICR5	0x80
@@ -57,6 +56,12 @@ struct aspeed_lpc_snoop_model_data {
 	unsigned int has_hicrb_ensnp;
 };
 
+enum aspeed_lpc_snoop_index {
+	ASPEED_LPC_SNOOP_INDEX_0 = 0,
+	ASPEED_LPC_SNOOP_INDEX_1 = 1,
+	ASPEED_LPC_SNOOP_INDEX_MAX = ASPEED_LPC_SNOOP_INDEX_1,
+};
+
 struct aspeed_lpc_snoop_channel {
 	bool enabled;
 	struct kfifo		fifo;
@@ -68,7 +73,7 @@ struct aspeed_lpc_snoop {
 	struct regmap		*regmap;
 	int			irq;
 	struct clk		*clk;
-	struct aspeed_lpc_snoop_channel chan[NUM_SNOOP_CHANNELS];
+	struct aspeed_lpc_snoop_channel chan[ASPEED_LPC_SNOOP_INDEX_MAX + 1];
 };
 
 static struct aspeed_lpc_snoop_channel *snoop_file_to_chan(struct file *file)
@@ -182,9 +187,10 @@ static int aspeed_lpc_snoop_config_irq(struct aspeed_lpc_snoop *lpc_snoop,
 	return 0;
 }
 
+__attribute__((nonnull))
 static int aspeed_lpc_enable_snoop(struct aspeed_lpc_snoop *lpc_snoop,
 				   struct device *dev,
-				   int channel, u16 lpc_port)
+				   enum aspeed_lpc_snoop_index channel, u16 lpc_port)
 {
 	const struct aspeed_lpc_snoop_model_data *model_data;
 	u32 hicr5_en, snpwadr_mask, snpwadr_shift, hicrb_en;
@@ -251,8 +257,9 @@ static int aspeed_lpc_enable_snoop(struct aspeed_lpc_snoop *lpc_snoop,
 	return rc;
 }
 
+__attribute__((nonnull))
 static void aspeed_lpc_disable_snoop(struct aspeed_lpc_snoop *lpc_snoop,
-				     int channel)
+				     enum aspeed_lpc_snoop_index channel)
 {
 	if (!lpc_snoop->chan[channel].enabled)
 		return;
@@ -331,16 +338,16 @@ static int aspeed_lpc_snoop_probe(struct platform_device *pdev)
 	if (rc)
 		goto err;
 
-	rc = aspeed_lpc_enable_snoop(lpc_snoop, dev, 0, port);
+	rc = aspeed_lpc_enable_snoop(lpc_snoop, dev, ASPEED_LPC_SNOOP_INDEX_0, port);
 	if (rc)
 		goto err;
 
 	/* Configuration of 2nd snoop channel port is optional */
 	if (of_property_read_u32_index(dev->of_node, "snoop-ports",
 				       1, &port) == 0) {
-		rc = aspeed_lpc_enable_snoop(lpc_snoop, dev, 1, port);
+		rc = aspeed_lpc_enable_snoop(lpc_snoop, dev, ASPEED_LPC_SNOOP_INDEX_1, port);
 		if (rc) {
-			aspeed_lpc_disable_snoop(lpc_snoop, 0);
+			aspeed_lpc_disable_snoop(lpc_snoop, ASPEED_LPC_SNOOP_INDEX_0);
 			goto err;
 		}
 	}
@@ -358,8 +365,8 @@ static void aspeed_lpc_snoop_remove(struct platform_device *pdev)
 	struct aspeed_lpc_snoop *lpc_snoop = dev_get_drvdata(&pdev->dev);
 
 	/* Disable both snoop channels */
-	aspeed_lpc_disable_snoop(lpc_snoop, 0);
-	aspeed_lpc_disable_snoop(lpc_snoop, 1);
+	aspeed_lpc_disable_snoop(lpc_snoop, ASPEED_LPC_SNOOP_INDEX_0);
+	aspeed_lpc_disable_snoop(lpc_snoop, ASPEED_LPC_SNOOP_INDEX_1);
 
 	clk_disable_unprepare(lpc_snoop->clk);
 }

-- 
2.39.5
Re: [PATCH v2 04/10] soc: aspeed: lpc-snoop: Constrain parameters in channel paths
Posted by Jean Delvare 3 months, 1 week ago 
On Mon, 16 Jun 2025 22:43:41 +0930, Andrew Jeffery wrote:
> Ensure pointers and the channel index are valid before use.
> 
> Signed-off-by: Andrew Jeffery <andrew@codeconstruct.com.au>
> ---
>  drivers/soc/aspeed/aspeed-lpc-snoop.c | 25 ++++++++++++++++---------
>  1 file changed, 16 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/soc/aspeed/aspeed-lpc-snoop.c b/drivers/soc/aspeed/aspeed-lpc-snoop.c
> index ca7536213e0986f737606a52996ffea620df2a7a..804c6ed9c4c671da73a6c66c1de41c59922c82dc 100644
> --- a/drivers/soc/aspeed/aspeed-lpc-snoop.c
> +++ b/drivers/soc/aspeed/aspeed-lpc-snoop.c
> @@ -25,7 +25,6 @@
>  
>  #define DEVICE_NAME	"aspeed-lpc-snoop"
>  
> -#define NUM_SNOOP_CHANNELS 2
>  #define SNOOP_FIFO_SIZE 2048
>  
>  #define HICR5	0x80
> @@ -57,6 +56,12 @@ struct aspeed_lpc_snoop_model_data {
>  	unsigned int has_hicrb_ensnp;
>  };
>  
> +enum aspeed_lpc_snoop_index {
> +	ASPEED_LPC_SNOOP_INDEX_0 = 0,
> +	ASPEED_LPC_SNOOP_INDEX_1 = 1,
> +	ASPEED_LPC_SNOOP_INDEX_MAX = ASPEED_LPC_SNOOP_INDEX_1,
> +};

I don't have a strong opinion on this (again, I'm neither the driver
maintainer nor the subsystem maintainer so my opinion has little
value), but IMHO the main value of introducing an enum here was to make
it possible to get rid of the default statement in the switch
constructs. With switch constructs being gone in patch 10/10 (soc:
aspeed: lpc-snoop: Lift channel config to const structs), the value of
this enum seems pretty low now. You could use NUM_SNOOP_CHANNELS
instead of ASPEED_LPC_SNOOP_INDEX_MAX + 1 and 0 and 1 instead of
ASPEED_LPC_SNOOP_INDEX_0 and ASPEED_LPC_SNOOP_INDEX_1, respectively,
and the code would work just the same, while being more simple, with no
downside that I can see.

-- 
Jean Delvare
SUSE L3 Support
Re: [PATCH v2 04/10] soc: aspeed: lpc-snoop: Constrain parameters in channel paths
Posted by Andrew Jeffery 3 months ago 
Hi Jean,

On Fri, 2025-07-04 at 18:44 +0200, Jean Delvare wrote:
> On Mon, 16 Jun 2025 22:43:41 +0930, Andrew Jeffery wrote:
> > Ensure pointers and the channel index are valid before use.
> > 
> > Signed-off-by: Andrew Jeffery <andrew@codeconstruct.com.au>
> > ---
> >  drivers/soc/aspeed/aspeed-lpc-snoop.c | 25 ++++++++++++++++---------
> >  1 file changed, 16 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/soc/aspeed/aspeed-lpc-snoop.c b/drivers/soc/aspeed/aspeed-lpc-snoop.c
> > index ca7536213e0986f737606a52996ffea620df2a7a..804c6ed9c4c671da73a6c66c1de41c59922c82dc 100644
> > --- a/drivers/soc/aspeed/aspeed-lpc-snoop.c
> > +++ b/drivers/soc/aspeed/aspeed-lpc-snoop.c
> > @@ -25,7 +25,6 @@
> >  
> >  #define DEVICE_NAME    "aspeed-lpc-snoop"
> >  
> > -#define NUM_SNOOP_CHANNELS 2
> >  #define SNOOP_FIFO_SIZE 2048
> >  
> >  #define HICR5  0x80
> > @@ -57,6 +56,12 @@ struct aspeed_lpc_snoop_model_data {
> >         unsigned int has_hicrb_ensnp;
> >  };
> >  
> > +enum aspeed_lpc_snoop_index {
> > +       ASPEED_LPC_SNOOP_INDEX_0 = 0,
> > +       ASPEED_LPC_SNOOP_INDEX_1 = 1,
> > +       ASPEED_LPC_SNOOP_INDEX_MAX = ASPEED_LPC_SNOOP_INDEX_1,
> > +};
> 
> I don't have a strong opinion on this (again, I'm neither the driver
> maintainer nor the subsystem maintainer so my opinion has little
> value), but IMHO the main value of introducing an enum here was to make
> it possible to get rid of the default statement in the switch
> constructs. With switch constructs being gone in patch 10/10 (soc:
> aspeed: lpc-snoop: Lift channel config to const structs), the value of
> this enum seems pretty low now. You could use NUM_SNOOP_CHANNELS
> instead of ASPEED_LPC_SNOOP_INDEX_MAX + 1 and 0 and 1 instead of
> ASPEED_LPC_SNOOP_INDEX_0 and ASPEED_LPC_SNOOP_INDEX_1, respectively,
> and the code would work just the same, while being more simple, with no
> downside that I can see.
> 

Yeah, I agonised over it a bit before posting. However, I'm on leave,
and I'd like to draw a line under this series. This patch is in the
middle of it, and I'd rather not disrupt it too much and go around
again with a v3. I'm going to keep the enum for now, but if I need to
tidy up the driver down again the track I'll reconsider its worth.

Andrew

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.