[PATCH 0/7] soc: aspeed: lpc-snoop: Miscellaneous fixes

[PATCH 0/7] soc: aspeed: lpc-snoop: Miscellaneous fixes

[Andrew Jeffery]

Henry's bug[1] and fix[2] prompted some further inspection by
Jean.

This series provides fixes for the remaining issues Jean identified, as
well as reworking the channel paths to reduce cleanup required in error
paths. It is based on the tree at[3].

Lightly tested on an AST2600 EVB. Further testing on platforms
designed around the snoop device appreciated.

[1]: https://bugzilla.kernel.org/show_bug.cgi?id=219934
[2]: https://lore.kernel.org/all/20250401074647.21300-1-bsdhenrymartin@gmail.com/
[3]: https://git.kernel.org/pub/scm/linux/kernel/git/arj/bmc.git/log/?h=aspeed/drivers

Signed-off-by: Andrew Jeffery <andrew@codeconstruct.com.au>
---
Andrew Jeffery (7):
      soc: aspeed: lpc-snoop: Cleanup resources in stack-order
      soc: aspeed: lpc-snoop: Don't disable channels that aren't enabled
      soc: aspeed: lpc-snoop: Ensure model_data is valid
      soc: aspeed: lpc-snoop: Constrain parameters in channel paths
      soc: aspeed: lpc-snoop: Rename 'channel' to 'index' in channel paths
      soc: aspeed: lpc-snoop: Rearrange channel paths
      soc: aspeed: lpc-snoop: Lift channel config to const structs

 drivers/soc/aspeed/aspeed-lpc-snoop.c | 149 ++++++++++++++++++++--------------
 1 file changed, 88 insertions(+), 61 deletions(-)
---
base-commit: f3089a4fc24777ea2fccdf4ffc84732b1da65bdc
change-id: 20250401-aspeed-lpc-snoop-fixes-e5d2883da3a3

Best regards,
-- 
Andrew Jeffery <andrew@codeconstruct.com.au>

[PATCH 1/7] soc: aspeed: lpc-snoop: Cleanup resources in stack-order

[Andrew Jeffery]

Free the kfifo after unregistering the miscdev in
aspeed_lpc_disable_snoop() as the kfifo is initialised before the
miscdev in aspeed_lpc_enable_snoop().

Fixes: 3772e5da4454 ("drivers/misc: Aspeed LPC snoop output using misc chardev")
Cc: stable@vger.kernel.org
Cc: Jean Delvare <jdelvare@suse.de>
Signed-off-by: Andrew Jeffery <andrew@codeconstruct.com.au>
---
 drivers/soc/aspeed/aspeed-lpc-snoop.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/soc/aspeed/aspeed-lpc-snoop.c b/drivers/soc/aspeed/aspeed-lpc-snoop.c
index 3e3f178b122615b33e10ff25a0b0fe7b40a0b667..bfa770ec51a889260d11c26e675f3320bf710a54 100644
--- a/drivers/soc/aspeed/aspeed-lpc-snoop.c
+++ b/drivers/soc/aspeed/aspeed-lpc-snoop.c
@@ -263,8 +263,8 @@ static void aspeed_lpc_disable_snoop(struct aspeed_lpc_snoop *lpc_snoop,
 		return;
 	}
 
-	kfifo_free(&lpc_snoop->chan[channel].fifo);
 	misc_deregister(&lpc_snoop->chan[channel].miscdev);
+	kfifo_free(&lpc_snoop->chan[channel].fifo);
 }
 
 static int aspeed_lpc_snoop_probe(struct platform_device *pdev)

-- 
2.39.5

[PATCH 2/7] soc: aspeed: lpc-snoop: Don't disable channels that aren't enabled

[Andrew Jeffery]

Mitigate e.g. the following:

    # echo 1e789080.lpc-snoop > /sys/bus/platform/drivers/aspeed-lpc-snoop/unbind
    ...
    [  120.363594] Unable to handle kernel NULL pointer dereference at virtual address 00000004 when write
    [  120.373866] [00000004] *pgd=00000000
    [  120.377910] Internal error: Oops: 805 [#1] SMP ARM
    [  120.383306] CPU: 1 UID: 0 PID: 315 Comm: sh Not tainted 6.15.0-rc1-00009-g926217bc7d7d-dirty #20 NONE
    ...
    [  120.679543] Call trace:
    [  120.679559]  misc_deregister from aspeed_lpc_snoop_remove+0x84/0xac
    [  120.692462]  aspeed_lpc_snoop_remove from platform_remove+0x28/0x38
    [  120.700996]  platform_remove from device_release_driver_internal+0x188/0x200
    ...

Fixes: 9f4f9ae81d0a ("drivers/misc: add Aspeed LPC snoop driver")
Cc: stable@vger.kernel.org
Cc: Jean Delvare <jdelvare@suse.de>
Signed-off-by: Andrew Jeffery <andrew@codeconstruct.com.au>
---
 drivers/soc/aspeed/aspeed-lpc-snoop.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/soc/aspeed/aspeed-lpc-snoop.c b/drivers/soc/aspeed/aspeed-lpc-snoop.c
index bfa770ec51a889260d11c26e675f3320bf710a54..e9d9a8e60a6f062c0b53c9c02e5d73768453998d 100644
--- a/drivers/soc/aspeed/aspeed-lpc-snoop.c
+++ b/drivers/soc/aspeed/aspeed-lpc-snoop.c
@@ -58,6 +58,7 @@ struct aspeed_lpc_snoop_model_data {
 };
 
 struct aspeed_lpc_snoop_channel {
+	bool enabled;
 	struct kfifo		fifo;
 	wait_queue_head_t	wq;
 	struct miscdevice	miscdev;
@@ -190,6 +191,9 @@ static int aspeed_lpc_enable_snoop(struct aspeed_lpc_snoop *lpc_snoop,
 	const struct aspeed_lpc_snoop_model_data *model_data =
 		of_device_get_match_data(dev);
 
+	if (lpc_snoop->chan[channel].enabled)
+		return -EBUSY;
+
 	init_waitqueue_head(&lpc_snoop->chan[channel].wq);
 	/* Create FIFO datastructure */
 	rc = kfifo_alloc(&lpc_snoop->chan[channel].fifo,
@@ -236,6 +240,8 @@ static int aspeed_lpc_enable_snoop(struct aspeed_lpc_snoop *lpc_snoop,
 		regmap_update_bits(lpc_snoop->regmap, HICRB,
 				hicrb_en, hicrb_en);
 
+	lpc_snoop->chan[channel].enabled = true;
+
 	return 0;
 
 err_misc_deregister:
@@ -248,6 +254,9 @@ static int aspeed_lpc_enable_snoop(struct aspeed_lpc_snoop *lpc_snoop,
 static void aspeed_lpc_disable_snoop(struct aspeed_lpc_snoop *lpc_snoop,
 				     int channel)
 {
+	if (!lpc_snoop->chan[channel].enabled)
+		return;
+
 	switch (channel) {
 	case 0:
 		regmap_update_bits(lpc_snoop->regmap, HICR5,
@@ -263,6 +272,8 @@ static void aspeed_lpc_disable_snoop(struct aspeed_lpc_snoop *lpc_snoop,
 		return;
 	}
 
+	lpc_snoop->chan[channel].enabled = false;
+	/* Consider improving safety wrt concurrent reader(s) */
 	misc_deregister(&lpc_snoop->chan[channel].miscdev);
 	kfifo_free(&lpc_snoop->chan[channel].fifo);
 }

-- 
2.39.5

[PATCH 3/7] soc: aspeed: lpc-snoop: Ensure model_data is valid

[Andrew Jeffery]

of_device_get_match_data() can return NULL, though shouldn't in current
circumstances. Regardless, initialise model_data closer to use so it's
clear we need to test for validity prior to dereferencing.

Fixes: 2dee584bc9e3 ("drivers/misc: (aspeed-lpc-snoop): Add ast2400 to compat")
Signed-off-by: Andrew Jeffery <andrew@codeconstruct.com.au>
---
 drivers/soc/aspeed/aspeed-lpc-snoop.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/soc/aspeed/aspeed-lpc-snoop.c b/drivers/soc/aspeed/aspeed-lpc-snoop.c
index e9d9a8e60a6f062c0b53c9c02e5d73768453998d..28f034b8a3b7226efe20cbe30a7da0c2b49fbd96 100644
--- a/drivers/soc/aspeed/aspeed-lpc-snoop.c
+++ b/drivers/soc/aspeed/aspeed-lpc-snoop.c
@@ -186,10 +186,10 @@ static int aspeed_lpc_enable_snoop(struct aspeed_lpc_snoop *lpc_snoop,
 				   struct device *dev,
 				   int channel, u16 lpc_port)
 {
-	int rc = 0;
+	const struct aspeed_lpc_snoop_model_data *model_data;
 	u32 hicr5_en, snpwadr_mask, snpwadr_shift, hicrb_en;
-	const struct aspeed_lpc_snoop_model_data *model_data =
-		of_device_get_match_data(dev);
+	int rc = 0;
+
 
 	if (lpc_snoop->chan[channel].enabled)
 		return -EBUSY;
@@ -236,9 +236,10 @@ static int aspeed_lpc_enable_snoop(struct aspeed_lpc_snoop *lpc_snoop,
 	regmap_update_bits(lpc_snoop->regmap, HICR5, hicr5_en, hicr5_en);
 	regmap_update_bits(lpc_snoop->regmap, SNPWADR, snpwadr_mask,
 			   lpc_port << snpwadr_shift);
-	if (model_data->has_hicrb_ensnp)
-		regmap_update_bits(lpc_snoop->regmap, HICRB,
-				hicrb_en, hicrb_en);
+
+	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);
 
 	lpc_snoop->chan[channel].enabled = true;
 

-- 
2.39.5

[PATCH 4/7] soc: aspeed: lpc-snoop: Constrain parameters in channel paths

[Andrew Jeffery]

Ensure pointers and the channel index are valid before use.

Fixes: 9f4f9ae81d0a ("drivers/misc: add Aspeed LPC snoop driver")
Signed-off-by: Andrew Jeffery <andrew@codeconstruct.com.au>
---
 drivers/soc/aspeed/aspeed-lpc-snoop.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/soc/aspeed/aspeed-lpc-snoop.c b/drivers/soc/aspeed/aspeed-lpc-snoop.c
index 28f034b8a3b7226efe20cbe30a7da0c2b49fbd96..6ab362aeb180c8ad356422d8257717f41a232b3c 100644
--- a/drivers/soc/aspeed/aspeed-lpc-snoop.c
+++ b/drivers/soc/aspeed/aspeed-lpc-snoop.c
@@ -182,6 +182,7 @@ 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)
@@ -190,6 +191,8 @@ static int aspeed_lpc_enable_snoop(struct aspeed_lpc_snoop *lpc_snoop,
 	u32 hicr5_en, snpwadr_mask, snpwadr_shift, hicrb_en;
 	int rc = 0;
 
+	if (channel < 0 || channel >= ARRAY_SIZE(lpc_snoop->chan))
+		return -EINVAL;
 
 	if (lpc_snoop->chan[channel].enabled)
 		return -EBUSY;
@@ -252,9 +255,13 @@ 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)
 {
+	if (channel < 0 || channel >= ARRAY_SIZE(lpc_snoop->chan))
+		return;
+
 	if (!lpc_snoop->chan[channel].enabled)
 		return;
 

-- 
2.39.5

[PATCH 5/7] soc: aspeed: lpc-snoop: Rename 'channel' to 'index' in channel paths

[Andrew Jeffery]

We'll introduce another 'channel' variable shortly

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

diff --git a/drivers/soc/aspeed/aspeed-lpc-snoop.c b/drivers/soc/aspeed/aspeed-lpc-snoop.c
index 6ab362aeb180c8ad356422d8257717f41a232b3c..f6952f71eda52c95aea5ad1084edd218b88f1234 100644
--- a/drivers/soc/aspeed/aspeed-lpc-snoop.c
+++ b/drivers/soc/aspeed/aspeed-lpc-snoop.c
@@ -185,40 +185,40 @@ static int aspeed_lpc_snoop_config_irq(struct aspeed_lpc_snoop *lpc_snoop,
 __attribute__((nonnull))
 static int aspeed_lpc_enable_snoop(struct aspeed_lpc_snoop *lpc_snoop,
 				   struct device *dev,
-				   int channel, u16 lpc_port)
+				   int index, u16 lpc_port)
 {
 	const struct aspeed_lpc_snoop_model_data *model_data;
 	u32 hicr5_en, snpwadr_mask, snpwadr_shift, hicrb_en;
 	int rc = 0;
 
-	if (channel < 0 || channel >= ARRAY_SIZE(lpc_snoop->chan))
+	if (index < 0 || index >= ARRAY_SIZE(lpc_snoop->chan))
 		return -EINVAL;
 
-	if (lpc_snoop->chan[channel].enabled)
+	if (lpc_snoop->chan[index].enabled)
 		return -EBUSY;
 
-	init_waitqueue_head(&lpc_snoop->chan[channel].wq);
+	init_waitqueue_head(&lpc_snoop->chan[index].wq);
 	/* Create FIFO datastructure */
-	rc = kfifo_alloc(&lpc_snoop->chan[channel].fifo,
+	rc = kfifo_alloc(&lpc_snoop->chan[index].fifo,
 			 SNOOP_FIFO_SIZE, GFP_KERNEL);
 	if (rc)
 		return rc;
 
-	lpc_snoop->chan[channel].miscdev.minor = MISC_DYNAMIC_MINOR;
-	lpc_snoop->chan[channel].miscdev.name =
-		devm_kasprintf(dev, GFP_KERNEL, "%s%d", DEVICE_NAME, channel);
-	if (!lpc_snoop->chan[channel].miscdev.name) {
+	lpc_snoop->chan[index].miscdev.minor = MISC_DYNAMIC_MINOR;
+	lpc_snoop->chan[index].miscdev.name =
+		devm_kasprintf(dev, GFP_KERNEL, "%s%d", DEVICE_NAME, index);
+	if (!lpc_snoop->chan[index].miscdev.name) {
 		rc = -ENOMEM;
 		goto err_free_fifo;
 	}
-	lpc_snoop->chan[channel].miscdev.fops = &snoop_fops;
-	lpc_snoop->chan[channel].miscdev.parent = dev;
-	rc = misc_register(&lpc_snoop->chan[channel].miscdev);
+	lpc_snoop->chan[index].miscdev.fops = &snoop_fops;
+	lpc_snoop->chan[index].miscdev.parent = dev;
+	rc = misc_register(&lpc_snoop->chan[index].miscdev);
 	if (rc)
 		goto err_free_fifo;
 
 	/* Enable LPC snoop channel at requested port */
-	switch (channel) {
+	switch (index) {
 	case 0:
 		hicr5_en = HICR5_EN_SNP0W | HICR5_ENINT_SNP0W;
 		snpwadr_mask = SNPWADR_CH0_MASK;
@@ -244,28 +244,29 @@ static int aspeed_lpc_enable_snoop(struct aspeed_lpc_snoop *lpc_snoop,
 	if (model_data && model_data->has_hicrb_ensnp)
 		regmap_update_bits(lpc_snoop->regmap, HICRB, hicrb_en, hicrb_en);
 
-	lpc_snoop->chan[channel].enabled = true;
+	lpc_snoop->chan[index].enabled = true;
 
 	return 0;
 
 err_misc_deregister:
-	misc_deregister(&lpc_snoop->chan[channel].miscdev);
+	misc_deregister(&lpc_snoop->chan[index].miscdev);
 err_free_fifo:
-	kfifo_free(&lpc_snoop->chan[channel].fifo);
+	kfifo_free(&lpc_snoop->chan[index].fifo);
 	return rc;
 }
 
 __attribute__((nonnull))
 static void aspeed_lpc_disable_snoop(struct aspeed_lpc_snoop *lpc_snoop,
-				     int channel)
+				     int index)
 {
-	if (channel < 0 || channel >= ARRAY_SIZE(lpc_snoop->chan))
+	if (index < 0 || index >= ARRAY_SIZE(lpc_snoop->chan))
 		return;
 
-	if (!lpc_snoop->chan[channel].enabled)
+	if (!lpc_snoop->chan[index].enabled)
 		return;
 
-	switch (channel) {
+	/* Disable interrupts along with the device */
+	switch (index) {
 	case 0:
 		regmap_update_bits(lpc_snoop->regmap, HICR5,
 				   HICR5_EN_SNP0W | HICR5_ENINT_SNP0W,
@@ -280,10 +281,10 @@ static void aspeed_lpc_disable_snoop(struct aspeed_lpc_snoop *lpc_snoop,
 		return;
 	}
 
-	lpc_snoop->chan[channel].enabled = false;
+	lpc_snoop->chan[index].enabled = false;
 	/* Consider improving safety wrt concurrent reader(s) */
-	misc_deregister(&lpc_snoop->chan[channel].miscdev);
-	kfifo_free(&lpc_snoop->chan[channel].fifo);
+	misc_deregister(&lpc_snoop->chan[index].miscdev);
+	kfifo_free(&lpc_snoop->chan[index].fifo);
 }
 
 static int aspeed_lpc_snoop_probe(struct platform_device *pdev)

-- 
2.39.5

[PATCH 6/7] soc: aspeed: lpc-snoop: Rearrange channel paths

[Andrew Jeffery]

Order assignments such that tests for conditions not involving resource
acquisition are ordered before those testing acquired resources, and
order managed resource acquisition before unmanaged where possible. This
way we minimise the amount of manual cleanup required.

In the process, improve readability of the code by introducing a channel
pointer that takes the place of the repeated object lookups.

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

diff --git a/drivers/soc/aspeed/aspeed-lpc-snoop.c b/drivers/soc/aspeed/aspeed-lpc-snoop.c
index f6952f71eda52c95aea5ad1084edd218b88f1234..0b2044fd79b1be08dfa33bfcaf249b020c909bb9 100644
--- a/drivers/soc/aspeed/aspeed-lpc-snoop.c
+++ b/drivers/soc/aspeed/aspeed-lpc-snoop.c
@@ -189,31 +189,33 @@ static int aspeed_lpc_enable_snoop(struct aspeed_lpc_snoop *lpc_snoop,
 {
 	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;
 
 	if (index < 0 || index >= ARRAY_SIZE(lpc_snoop->chan))
 		return -EINVAL;
 
-	if (lpc_snoop->chan[index].enabled)
+	channel = &lpc_snoop->chan[index];
+
+	if (channel->enabled)
 		return -EBUSY;
 
-	init_waitqueue_head(&lpc_snoop->chan[index].wq);
-	/* Create FIFO datastructure */
-	rc = kfifo_alloc(&lpc_snoop->chan[index].fifo,
-			 SNOOP_FIFO_SIZE, GFP_KERNEL);
+	init_waitqueue_head(&channel->wq);
+
+	channel->miscdev.minor = MISC_DYNAMIC_MINOR;
+	channel->miscdev.fops = &snoop_fops;
+	channel->miscdev.parent = dev;
+
+	channel->miscdev.name =
+		devm_kasprintf(dev, GFP_KERNEL, "%s%d", DEVICE_NAME, index);
+	if (!channel->miscdev.name)
+		return -ENOMEM;
+
+	rc = kfifo_alloc(&channel->fifo, SNOOP_FIFO_SIZE, GFP_KERNEL);
 	if (rc)
 		return rc;
 
-	lpc_snoop->chan[index].miscdev.minor = MISC_DYNAMIC_MINOR;
-	lpc_snoop->chan[index].miscdev.name =
-		devm_kasprintf(dev, GFP_KERNEL, "%s%d", DEVICE_NAME, index);
-	if (!lpc_snoop->chan[index].miscdev.name) {
-		rc = -ENOMEM;
-		goto err_free_fifo;
-	}
-	lpc_snoop->chan[index].miscdev.fops = &snoop_fops;
-	lpc_snoop->chan[index].miscdev.parent = dev;
-	rc = misc_register(&lpc_snoop->chan[index].miscdev);
+	rc = misc_register(&channel->miscdev);
 	if (rc)
 		goto err_free_fifo;
 
@@ -236,6 +238,7 @@ static int aspeed_lpc_enable_snoop(struct aspeed_lpc_snoop *lpc_snoop,
 		goto err_misc_deregister;
 	}
 
+	/* Enable LPC snoop channel at requested port */
 	regmap_update_bits(lpc_snoop->regmap, HICR5, hicr5_en, hicr5_en);
 	regmap_update_bits(lpc_snoop->regmap, SNPWADR, snpwadr_mask,
 			   lpc_port << snpwadr_shift);
@@ -244,7 +247,7 @@ static int aspeed_lpc_enable_snoop(struct aspeed_lpc_snoop *lpc_snoop,
 	if (model_data && model_data->has_hicrb_ensnp)
 		regmap_update_bits(lpc_snoop->regmap, HICRB, hicrb_en, hicrb_en);
 
-	lpc_snoop->chan[index].enabled = true;
+	channel->enabled = true;
 
 	return 0;
 
@@ -259,10 +262,14 @@ __attribute__((nonnull))
 static void aspeed_lpc_disable_snoop(struct aspeed_lpc_snoop *lpc_snoop,
 				     int index)
 {
+	struct aspeed_lpc_snoop_channel *channel;
+
 	if (index < 0 || index >= ARRAY_SIZE(lpc_snoop->chan))
 		return;
 
-	if (!lpc_snoop->chan[index].enabled)
+	channel = &lpc_snoop->chan[index];
+
+	if (!channel->enabled)
 		return;
 
 	/* Disable interrupts along with the device */
@@ -281,10 +288,10 @@ static void aspeed_lpc_disable_snoop(struct aspeed_lpc_snoop *lpc_snoop,
 		return;
 	}
 
-	lpc_snoop->chan[index].enabled = false;
+	channel->enabled = false;
 	/* Consider improving safety wrt concurrent reader(s) */
-	misc_deregister(&lpc_snoop->chan[index].miscdev);
-	kfifo_free(&lpc_snoop->chan[index].fifo);
+	misc_deregister(&channel->miscdev);
+	kfifo_free(&channel->fifo);
 }
 
 static int aspeed_lpc_snoop_probe(struct platform_device *pdev)

-- 
2.39.5

[PATCH 7/7] soc: aspeed: lpc-snoop: Lift channel config to const structs

[Andrew Jeffery]

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.

Signed-off-by: Andrew Jeffery <andrew@codeconstruct.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))
+		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 */
-	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);
+	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);
 
 	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 */
-	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);
 
 	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");
+	lpc_snoop->chan[0].cfg = &channel_cfgs[0];
+	lpc_snoop->chan[1].cfg = &channel_cfgs[1];
+
 	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") &&

-- 
2.39.5

Re: [PATCH 1/7] soc: aspeed: lpc-snoop: Cleanup resources in stack-order

[Jean Delvare]

On Fri, 11 Apr 2025 10:38:31 +0930, Andrew Jeffery wrote:
> Free the kfifo after unregistering the miscdev in
> aspeed_lpc_disable_snoop() as the kfifo is initialised before the
> miscdev in aspeed_lpc_enable_snoop().
> 
> Fixes: 3772e5da4454 ("drivers/misc: Aspeed LPC snoop output using misc chardev")
> Cc: stable@vger.kernel.org
> Cc: Jean Delvare <jdelvare@suse.de>
> Signed-off-by: Andrew Jeffery <andrew@codeconstruct.com.au>
> ---
>  drivers/soc/aspeed/aspeed-lpc-snoop.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/soc/aspeed/aspeed-lpc-snoop.c b/drivers/soc/aspeed/aspeed-lpc-snoop.c
> index 3e3f178b122615b33e10ff25a0b0fe7b40a0b667..bfa770ec51a889260d11c26e675f3320bf710a54 100644
> --- a/drivers/soc/aspeed/aspeed-lpc-snoop.c
> +++ b/drivers/soc/aspeed/aspeed-lpc-snoop.c
> @@ -263,8 +263,8 @@ static void aspeed_lpc_disable_snoop(struct aspeed_lpc_snoop *lpc_snoop,
>  		return;
>  	}
>  
> -	kfifo_free(&lpc_snoop->chan[channel].fifo);
>  	misc_deregister(&lpc_snoop->chan[channel].miscdev);
> +	kfifo_free(&lpc_snoop->chan[channel].fifo);
>  }
>  
>  static int aspeed_lpc_snoop_probe(struct platform_device *pdev)
> 

Acked-by: Jean Delvare <jdelvare@suse.de>

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 2/7] soc: aspeed: lpc-snoop: Don't disable channels that aren't enabled

[Jean Delvare]

On Fri, 11 Apr 2025 10:38:32 +0930, Andrew Jeffery wrote:
> Mitigate e.g. the following:
> 
>     # echo 1e789080.lpc-snoop > /sys/bus/platform/drivers/aspeed-lpc-snoop/unbind
>     ...
>     [  120.363594] Unable to handle kernel NULL pointer dereference at virtual address 00000004 when write
>     [  120.373866] [00000004] *pgd=00000000
>     [  120.377910] Internal error: Oops: 805 [#1] SMP ARM
>     [  120.383306] CPU: 1 UID: 0 PID: 315 Comm: sh Not tainted 6.15.0-rc1-00009-g926217bc7d7d-dirty #20 NONE
>     ...
>     [  120.679543] Call trace:
>     [  120.679559]  misc_deregister from aspeed_lpc_snoop_remove+0x84/0xac
>     [  120.692462]  aspeed_lpc_snoop_remove from platform_remove+0x28/0x38
>     [  120.700996]  platform_remove from device_release_driver_internal+0x188/0x200
>     ...
> 
> Fixes: 9f4f9ae81d0a ("drivers/misc: add Aspeed LPC snoop driver")
> Cc: stable@vger.kernel.org
> Cc: Jean Delvare <jdelvare@suse.de>
> Signed-off-by: Andrew Jeffery <andrew@codeconstruct.com.au>
> ---
>  drivers/soc/aspeed/aspeed-lpc-snoop.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/soc/aspeed/aspeed-lpc-snoop.c b/drivers/soc/aspeed/aspeed-lpc-snoop.c
> index bfa770ec51a889260d11c26e675f3320bf710a54..e9d9a8e60a6f062c0b53c9c02e5d73768453998d 100644
> --- a/drivers/soc/aspeed/aspeed-lpc-snoop.c
> +++ b/drivers/soc/aspeed/aspeed-lpc-snoop.c
> @@ -58,6 +58,7 @@ struct aspeed_lpc_snoop_model_data {
>  };
>  
>  struct aspeed_lpc_snoop_channel {
> +	bool enabled;
>  	struct kfifo		fifo;
>  	wait_queue_head_t	wq;
>  	struct miscdevice	miscdev;
> @@ -190,6 +191,9 @@ static int aspeed_lpc_enable_snoop(struct aspeed_lpc_snoop *lpc_snoop,
>  	const struct aspeed_lpc_snoop_model_data *model_data =
>  		of_device_get_match_data(dev);
>  
> +	if (lpc_snoop->chan[channel].enabled)
> +		return -EBUSY;

This isn't supposed to happen, right? WARN_ON() may be appropriate.

> +
>  	init_waitqueue_head(&lpc_snoop->chan[channel].wq);
>  	/* Create FIFO datastructure */
>  	rc = kfifo_alloc(&lpc_snoop->chan[channel].fifo,
> @@ -236,6 +240,8 @@ static int aspeed_lpc_enable_snoop(struct aspeed_lpc_snoop *lpc_snoop,
>  		regmap_update_bits(lpc_snoop->regmap, HICRB,
>  				hicrb_en, hicrb_en);
>  
> +	lpc_snoop->chan[channel].enabled = true;
> +
>  	return 0;
>  
>  err_misc_deregister:
> @@ -248,6 +254,9 @@ static int aspeed_lpc_enable_snoop(struct aspeed_lpc_snoop *lpc_snoop,
>  static void aspeed_lpc_disable_snoop(struct aspeed_lpc_snoop *lpc_snoop,
>  				     int channel)
>  {
> +	if (!lpc_snoop->chan[channel].enabled)
> +		return;
> +
>  	switch (channel) {
>  	case 0:
>  		regmap_update_bits(lpc_snoop->regmap, HICR5,
> @@ -263,6 +272,8 @@ static void aspeed_lpc_disable_snoop(struct aspeed_lpc_snoop *lpc_snoop,
>  		return;
>  	}
>  
> +	lpc_snoop->chan[channel].enabled = false;
> +	/* Consider improving safety wrt concurrent reader(s) */
>  	misc_deregister(&lpc_snoop->chan[channel].miscdev);
>  	kfifo_free(&lpc_snoop->chan[channel].fifo);
>  }
> 

Acked-by: Jean Delvare <jdelvare@suse.de>

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 3/7] soc: aspeed: lpc-snoop: Ensure model_data is valid

[Jean Delvare]

On Fri, 11 Apr 2025 10:38:33 +0930, Andrew Jeffery wrote:
> of_device_get_match_data() can return NULL, though shouldn't in current
> circumstances. Regardless, initialise model_data closer to use so it's
> clear we need to test for validity prior to dereferencing.
> 
> Fixes: 2dee584bc9e3 ("drivers/misc: (aspeed-lpc-snoop): Add ast2400 to compat")
> Signed-off-by: Andrew Jeffery <andrew@codeconstruct.com.au>
> ---
>  drivers/soc/aspeed/aspeed-lpc-snoop.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/soc/aspeed/aspeed-lpc-snoop.c b/drivers/soc/aspeed/aspeed-lpc-snoop.c
> index e9d9a8e60a6f062c0b53c9c02e5d73768453998d..28f034b8a3b7226efe20cbe30a7da0c2b49fbd96 100644
> --- a/drivers/soc/aspeed/aspeed-lpc-snoop.c
> +++ b/drivers/soc/aspeed/aspeed-lpc-snoop.c
> @@ -186,10 +186,10 @@ static int aspeed_lpc_enable_snoop(struct aspeed_lpc_snoop *lpc_snoop,
>  				   struct device *dev,
>  				   int channel, u16 lpc_port)
>  {
> -	int rc = 0;
> +	const struct aspeed_lpc_snoop_model_data *model_data;
>  	u32 hicr5_en, snpwadr_mask, snpwadr_shift, hicrb_en;
> -	const struct aspeed_lpc_snoop_model_data *model_data =
> -		of_device_get_match_data(dev);
> +	int rc = 0;
> +

Duplicate blank line.

>  
>  	if (lpc_snoop->chan[channel].enabled)
>  		return -EBUSY;
> @@ -236,9 +236,10 @@ static int aspeed_lpc_enable_snoop(struct aspeed_lpc_snoop *lpc_snoop,
>  	regmap_update_bits(lpc_snoop->regmap, HICR5, hicr5_en, hicr5_en);
>  	regmap_update_bits(lpc_snoop->regmap, SNPWADR, snpwadr_mask,
>  			   lpc_port << snpwadr_shift);
> -	if (model_data->has_hicrb_ensnp)
> -		regmap_update_bits(lpc_snoop->regmap, HICRB,
> -				hicrb_en, hicrb_en);
> +
> +	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);
>  
>  	lpc_snoop->chan[channel].enabled = true;
>  
> 

Acked-by: Jean Delvare <jdelvare@suse.de>

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 4/7] soc: aspeed: lpc-snoop: Constrain parameters in channel paths

[Jean Delvare]

On Fri, 11 Apr 2025 10:38:34 +0930, Andrew Jeffery wrote:
> Ensure pointers and the channel index are valid before use.
> 
> Fixes: 9f4f9ae81d0a ("drivers/misc: add Aspeed LPC snoop driver")

Please don't abuse Fixes tags. Here you are hardening the code just in
case, but this isn't fixing any actual bug, as functions
aspeed_lpc_enable_snoop() and aspeed_lpc_disable_snoop() were never
called with an incorrect channel index.

> Signed-off-by: Andrew Jeffery <andrew@codeconstruct.com.au>
> ---
>  drivers/soc/aspeed/aspeed-lpc-snoop.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/soc/aspeed/aspeed-lpc-snoop.c b/drivers/soc/aspeed/aspeed-lpc-snoop.c
> index 28f034b8a3b7226efe20cbe30a7da0c2b49fbd96..6ab362aeb180c8ad356422d8257717f41a232b3c 100644
> --- a/drivers/soc/aspeed/aspeed-lpc-snoop.c
> +++ b/drivers/soc/aspeed/aspeed-lpc-snoop.c
> @@ -182,6 +182,7 @@ 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)
> @@ -190,6 +191,8 @@ static int aspeed_lpc_enable_snoop(struct aspeed_lpc_snoop *lpc_snoop,
>  	u32 hicr5_en, snpwadr_mask, snpwadr_shift, hicrb_en;
>  	int rc = 0;
>  
> +	if (channel < 0 || channel >= ARRAY_SIZE(lpc_snoop->chan))
> +		return -EINVAL;
>  
>  	if (lpc_snoop->chan[channel].enabled)
>  		return -EBUSY;
> @@ -252,9 +255,13 @@ 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)
>  {
> +	if (channel < 0 || channel >= ARRAY_SIZE(lpc_snoop->chan))
> +		return;
> +
>  	if (!lpc_snoop->chan[channel].enabled)
>  		return;
>  
> 

TBH I'm not sure if this has much value, as any error in the channel
index (or passing NULL pointers for lpc_snoop or dev) would likely be
caught very early during driver development or update. And silently
returning early is not going to fix the problem if this ever happens.

But well, I'm not much into defensive programming anyway, so maybe this
is just me. As I'm not maintaining this driver, this ain't my decision.

Acked-by: Jean Delvare <jdelvare@suse.de>

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 5/7] soc: aspeed: lpc-snoop: Rename 'channel' to 'index' in channel paths

[Jean Delvare]

On Fri, 11 Apr 2025 10:38:35 +0930, Andrew Jeffery wrote:
> We'll introduce another 'channel' variable shortly
> 
> Signed-off-by: Andrew Jeffery <andrew@codeconstruct.com.au>
> ---
>  drivers/soc/aspeed/aspeed-lpc-snoop.c | 47 ++++++++++++++++++-----------------
>  1 file changed, 24 insertions(+), 23 deletions(-)
> (...)

Acked-by: Jean Delvare <jdelvare@suse.de>

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 2/7] soc: aspeed: lpc-snoop: Don't disable channels that aren't enabled

[Andrew Jeffery]

On Wed, 2025-04-16 at 14:15 +0200, Jean Delvare wrote:
> On Fri, 11 Apr 2025 10:38:32 +0930, Andrew Jeffery wrote:
> > Mitigate e.g. the following:
> > 
> >     # echo 1e789080.lpc-snoop > /sys/bus/platform/drivers/aspeed-
> > lpc-snoop/unbind
> >     ...
> >     [  120.363594] Unable to handle kernel NULL pointer dereference
> > at virtual address 00000004 when write
> >     [  120.373866] [00000004] *pgd=00000000
> >     [  120.377910] Internal error: Oops: 805 [#1] SMP ARM
> >     [  120.383306] CPU: 1 UID: 0 PID: 315 Comm: sh Not tainted
> > 6.15.0-rc1-00009-g926217bc7d7d-dirty #20 NONE
> >     ...
> >     [  120.679543] Call trace:
> >     [  120.679559]  misc_deregister from
> > aspeed_lpc_snoop_remove+0x84/0xac
> >     [  120.692462]  aspeed_lpc_snoop_remove from
> > platform_remove+0x28/0x38
> >     [  120.700996]  platform_remove from
> > device_release_driver_internal+0x188/0x200
> >     ...
> > 
> > Fixes: 9f4f9ae81d0a ("drivers/misc: add Aspeed LPC snoop driver")
> > Cc: stable@vger.kernel.org
> > Cc: Jean Delvare <jdelvare@suse.de>
> > Signed-off-by: Andrew Jeffery <andrew@codeconstruct.com.au>
> > ---
> >  drivers/soc/aspeed/aspeed-lpc-snoop.c | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> > 
> > diff --git a/drivers/soc/aspeed/aspeed-lpc-snoop.c
> > b/drivers/soc/aspeed/aspeed-lpc-snoop.c
> > index
> > bfa770ec51a889260d11c26e675f3320bf710a54..e9d9a8e60a6f062c0b53c9c02
> > e5d73768453998d 100644
> > --- a/drivers/soc/aspeed/aspeed-lpc-snoop.c
> > +++ b/drivers/soc/aspeed/aspeed-lpc-snoop.c
> > @@ -58,6 +58,7 @@ struct aspeed_lpc_snoop_model_data {
> >  };
> >  
> >  struct aspeed_lpc_snoop_channel {
> > +       bool enabled;
> >         struct kfifo            fifo;
> >         wait_queue_head_t       wq;
> >         struct miscdevice       miscdev;
> > @@ -190,6 +191,9 @@ static int aspeed_lpc_enable_snoop(struct
> > aspeed_lpc_snoop *lpc_snoop,
> >         const struct aspeed_lpc_snoop_model_data *model_data =
> >                 of_device_get_match_data(dev);
> >  
> > +       if (lpc_snoop->chan[channel].enabled)
> > +               return -EBUSY;
> 
> This isn't supposed to happen, right? 

No, not supposed to happen.

> WARN_ON() may be appropriate.

Ack.

> 
> > +
> >         init_waitqueue_head(&lpc_snoop->chan[channel].wq);
> >         /* Create FIFO datastructure */
> >         rc = kfifo_alloc(&lpc_snoop->chan[channel].fifo,
> > @@ -236,6 +240,8 @@ static int aspeed_lpc_enable_snoop(struct
> > aspeed_lpc_snoop *lpc_snoop,
> >                 regmap_update_bits(lpc_snoop->regmap, HICRB,
> >                                 hicrb_en, hicrb_en);
> >  
> > +       lpc_snoop->chan[channel].enabled = true;
> > +
> >         return 0;
> >  
> >  err_misc_deregister:
> > @@ -248,6 +254,9 @@ static int aspeed_lpc_enable_snoop(struct
> > aspeed_lpc_snoop *lpc_snoop,
> >  static void aspeed_lpc_disable_snoop(struct aspeed_lpc_snoop
> > *lpc_snoop,
> >                                      int channel)
> >  {
> > +       if (!lpc_snoop->chan[channel].enabled)
> > +               return;
> > +
> >         switch (channel) {
> >         case 0:
> >                 regmap_update_bits(lpc_snoop->regmap, HICR5,
> > @@ -263,6 +272,8 @@ static void aspeed_lpc_disable_snoop(struct
> > aspeed_lpc_snoop *lpc_snoop,
> >                 return;
> >         }
> >  
> > +       lpc_snoop->chan[channel].enabled = false;
> > +       /* Consider improving safety wrt concurrent reader(s) */
> >         misc_deregister(&lpc_snoop->chan[channel].miscdev);
> >         kfifo_free(&lpc_snoop->chan[channel].fifo);
> >  }
> > 
> 
> Acked-by: Jean Delvare <jdelvare@suse.de>
> 

Thanks,

Andrew

Re: [PATCH 3/7] soc: aspeed: lpc-snoop: Ensure model_data is valid

[Andrew Jeffery]

On Wed, 2025-04-16 at 14:19 +0200, Jean Delvare wrote:
> On Fri, 11 Apr 2025 10:38:33 +0930, Andrew Jeffery wrote:
> > of_device_get_match_data() can return NULL, though shouldn't in
> > current
> > circumstances. Regardless, initialise model_data closer to use so
> > it's
> > clear we need to test for validity prior to dereferencing.
> > 
> > Fixes: 2dee584bc9e3 ("drivers/misc: (aspeed-lpc-snoop): Add ast2400
> > to compat")
> > Signed-off-by: Andrew Jeffery <andrew@codeconstruct.com.au>
> > ---
> >  drivers/soc/aspeed/aspeed-lpc-snoop.c | 13 +++++++------
> >  1 file changed, 7 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/soc/aspeed/aspeed-lpc-snoop.c
> > b/drivers/soc/aspeed/aspeed-lpc-snoop.c
> > index
> > e9d9a8e60a6f062c0b53c9c02e5d73768453998d..28f034b8a3b7226efe20cbe30
> > a7da0c2b49fbd96 100644
> > --- a/drivers/soc/aspeed/aspeed-lpc-snoop.c
> > +++ b/drivers/soc/aspeed/aspeed-lpc-snoop.c
> > @@ -186,10 +186,10 @@ static int aspeed_lpc_enable_snoop(struct
> > aspeed_lpc_snoop *lpc_snoop,
> >                                    struct device *dev,
> >                                    int channel, u16 lpc_port)
> >  {
> > -       int rc = 0;
> > +       const struct aspeed_lpc_snoop_model_data *model_data;
> >         u32 hicr5_en, snpwadr_mask, snpwadr_shift, hicrb_en;
> > -       const struct aspeed_lpc_snoop_model_data *model_data =
> > -               of_device_get_match_data(dev);
> > +       int rc = 0;
> > +
> 
> Duplicate blank line.

I'll clean that up.

> 
> >  
> >         if (lpc_snoop->chan[channel].enabled)
> >                 return -EBUSY;
> > @@ -236,9 +236,10 @@ static int aspeed_lpc_enable_snoop(struct
> > aspeed_lpc_snoop *lpc_snoop,
> >         regmap_update_bits(lpc_snoop->regmap, HICR5, hicr5_en,
> > hicr5_en);
> >         regmap_update_bits(lpc_snoop->regmap, SNPWADR,
> > snpwadr_mask,
> >                            lpc_port << snpwadr_shift);
> > -       if (model_data->has_hicrb_ensnp)
> > -               regmap_update_bits(lpc_snoop->regmap, HICRB,
> > -                               hicrb_en, hicrb_en);
> > +
> > +       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);
> >  
> >         lpc_snoop->chan[channel].enabled = true;
> >  
> > 
> 
> Acked-by: Jean Delvare <jdelvare@suse.de>
> 

Thanks,

Andrew

Re: [PATCH 4/7] soc: aspeed: lpc-snoop: Constrain parameters in channel paths

[Andrew Jeffery]

On Wed, 2025-04-16 at 14:37 +0200, Jean Delvare wrote:
> On Fri, 11 Apr 2025 10:38:34 +0930, Andrew Jeffery wrote:
> > Ensure pointers and the channel index are valid before use.
> > 
> > Fixes: 9f4f9ae81d0a ("drivers/misc: add Aspeed LPC snoop driver")
> 
> Please don't abuse Fixes tags. Here you are hardening the code just in
> case, but this isn't fixing any actual bug, as functions
> aspeed_lpc_enable_snoop() and aspeed_lpc_disable_snoop() were never
> called with an incorrect channel index.

I'll drop the tag.

> 
> > Signed-off-by: Andrew Jeffery <andrew@codeconstruct.com.au>
> > ---
> >  drivers/soc/aspeed/aspeed-lpc-snoop.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/drivers/soc/aspeed/aspeed-lpc-snoop.c b/drivers/soc/aspeed/aspeed-lpc-snoop.c
> > index 28f034b8a3b7226efe20cbe30a7da0c2b49fbd96..6ab362aeb180c8ad356422d8257717f41a232b3c 100644
> > --- a/drivers/soc/aspeed/aspeed-lpc-snoop.c
> > +++ b/drivers/soc/aspeed/aspeed-lpc-snoop.c
> > @@ -182,6 +182,7 @@ 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)
> > @@ -190,6 +191,8 @@ static int aspeed_lpc_enable_snoop(struct aspeed_lpc_snoop *lpc_snoop,
> >         u32 hicr5_en, snpwadr_mask, snpwadr_shift, hicrb_en;
> >         int rc = 0;
> >  
> > +       if (channel < 0 || channel >= ARRAY_SIZE(lpc_snoop->chan))
> > +               return -EINVAL;
> >  
> >         if (lpc_snoop->chan[channel].enabled)
> >                 return -EBUSY;
> > @@ -252,9 +255,13 @@ 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)
> >  {
> > +       if (channel < 0 || channel >= ARRAY_SIZE(lpc_snoop->chan))
> > +               return;
> > +
> >         if (!lpc_snoop->chan[channel].enabled)
> >                 return;
> >  
> > 
> 
> TBH I'm not sure if this has much value, as any error in the channel
> index (or passing NULL pointers for lpc_snoop or dev) would likely be
> caught very early during driver development or update. And silently
> returning early is not going to fix the problem if this ever happens.
> 
> But well, I'm not much into defensive programming anyway, so maybe this
> is just me. As I'm not maintaining this driver, this ain't my decision.
> 
> Acked-by: Jean Delvare <jdelvare@suse.de>
> 

Given there's some other minor cleanups in your review of the series
I'll reconsider this approach as well.

Andrew

Re: [PATCH 6/7] soc: aspeed: lpc-snoop: Rearrange channel paths

[Jean Delvare]

On Fri, 11 Apr 2025 10:38:36 +0930, Andrew Jeffery wrote:
> Order assignments such that tests for conditions not involving resource
> acquisition are ordered before those testing acquired resources, and
> order managed resource acquisition before unmanaged where possible. This
> way we minimise the amount of manual cleanup required.
> 
> In the process, improve readability of the code by introducing a channel
> pointer that takes the place of the repeated object lookups.
> 
> Signed-off-by: Andrew Jeffery <andrew@codeconstruct.com.au>
> ---
>  drivers/soc/aspeed/aspeed-lpc-snoop.c | 47 ++++++++++++++++++++---------------
>  1 file changed, 27 insertions(+), 20 deletions(-)
> (...)

LGTM.

Acked-by: Jean Delvare <jdelvare@suse.de>

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 7/7] soc: aspeed: lpc-snoop: Lift channel config to const structs

[Jean Delvare]

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@codeconstruct.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

Re: [PATCH 7/7] soc: aspeed: lpc-snoop: Lift channel config to const structs

[Andrew Jeffery]

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@codeconstruct.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