From 8481d59be606d2338dbfe14b04cdbd1a3402c150 Mon Sep 17 00:00:00 2001 From: Andrew Jeffery Date: Mon, 16 Jun 2025 22:43:38 +0930 Subject: [PATCH 01/10] soc: aspeed: lpc-snoop: Cleanup resources in stack-order 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 Acked-by: Jean Delvare Link: https://patch.msgid.link/20250616-aspeed-lpc-snoop-fixes-v2-1-3cdd59c934d3@codeconstruct.com.au Signed-off-by: Andrew Jeffery --- 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 ef8f355589a5..59c18afa649c 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) From 56448e78a6bb4e1a8528a0e2efe94eff0400c247 Mon Sep 17 00:00:00 2001 From: Andrew Jeffery Date: Mon, 16 Jun 2025 22:43:39 +0930 Subject: [PATCH 02/10] soc: aspeed: lpc-snoop: Don't disable channels that aren't enabled 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 Acked-by: Jean Delvare Link: https://patch.msgid.link/20250616-aspeed-lpc-snoop-fixes-v2-2-3cdd59c934d3@codeconstruct.com.au Signed-off-by: Andrew Jeffery --- 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 59c18afa649c..fc3a2c41cc10 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 (WARN_ON(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); } From 3795e993931f2120bf64b8cdf03bb8c4f988b920 Mon Sep 17 00:00:00 2001 From: Andrew Jeffery Date: Mon, 16 Jun 2025 22:43:40 +0930 Subject: [PATCH 03/10] soc: aspeed: lpc-snoop: Ensure model_data is valid 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. Acked-by: Jean Delvare Link: https://patch.msgid.link/20250616-aspeed-lpc-snoop-fixes-v2-3-3cdd59c934d3@codeconstruct.com.au Signed-off-by: Andrew Jeffery --- drivers/soc/aspeed/aspeed-lpc-snoop.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/drivers/soc/aspeed/aspeed-lpc-snoop.c b/drivers/soc/aspeed/aspeed-lpc-snoop.c index fc3a2c41cc10..ca7536213e09 100644 --- a/drivers/soc/aspeed/aspeed-lpc-snoop.c +++ b/drivers/soc/aspeed/aspeed-lpc-snoop.c @@ -186,10 +186,9 @@ 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 (WARN_ON(lpc_snoop->chan[channel].enabled)) return -EBUSY; @@ -236,9 +235,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; From 3e9c15784a583ad242e3374839d1ce849876e11a Mon Sep 17 00:00:00 2001 From: Andrew Jeffery Date: Mon, 16 Jun 2025 22:43:41 +0930 Subject: [PATCH 04/10] soc: aspeed: lpc-snoop: Constrain parameters in channel paths Ensure pointers and the channel index are valid before use. Link: https://patch.msgid.link/20250616-aspeed-lpc-snoop-fixes-v2-4-3cdd59c934d3@codeconstruct.com.au Signed-off-by: Andrew Jeffery --- 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 ca7536213e09..804c6ed9c4c6 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); } From e88c9a712f9acc699ee69246d838bfca95956bf3 Mon Sep 17 00:00:00 2001 From: Andrew Jeffery Date: Mon, 16 Jun 2025 22:43:42 +0930 Subject: [PATCH 05/10] soc: aspeed: lpc-snoop: Rename 'channel' to 'index' in channel paths We'll introduce another 'channel' variable shortly Acked-by: Jean Delvare Link: https://patch.msgid.link/20250616-aspeed-lpc-snoop-fixes-v2-5-3cdd59c934d3@codeconstruct.com.au Signed-off-by: Andrew Jeffery --- drivers/soc/aspeed/aspeed-lpc-snoop.c | 43 ++++++++++++++------------- 1 file changed, 22 insertions(+), 21 deletions(-) diff --git a/drivers/soc/aspeed/aspeed-lpc-snoop.c b/drivers/soc/aspeed/aspeed-lpc-snoop.c index 804c6ed9c4c6..e9d17239163a 100644 --- a/drivers/soc/aspeed/aspeed-lpc-snoop.c +++ b/drivers/soc/aspeed/aspeed-lpc-snoop.c @@ -190,37 +190,37 @@ 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, - enum aspeed_lpc_snoop_index channel, u16 lpc_port) + enum aspeed_lpc_snoop_index 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 (WARN_ON(lpc_snoop->chan[channel].enabled)) + if (WARN_ON(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; @@ -246,25 +246,26 @@ 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, - enum aspeed_lpc_snoop_index channel) + enum aspeed_lpc_snoop_index index) { - 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, @@ -279,10 +280,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) From 6c64e1a828a20f841f3fcfe64bca6b1437d15f21 Mon Sep 17 00:00:00 2001 From: Andrew Jeffery Date: Mon, 16 Jun 2025 22:43:43 +0930 Subject: [PATCH 06/10] soc: aspeed: lpc-snoop: Rearrange channel paths 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. Acked-by: Jean Delvare Link: https://patch.msgid.link/20250616-aspeed-lpc-snoop-fixes-v2-6-3cdd59c934d3@codeconstruct.com.au Signed-off-by: Andrew Jeffery --- drivers/soc/aspeed/aspeed-lpc-snoop.c | 50 +++++++++++++++------------ 1 file changed, 28 insertions(+), 22 deletions(-) diff --git a/drivers/soc/aspeed/aspeed-lpc-snoop.c b/drivers/soc/aspeed/aspeed-lpc-snoop.c index e9d17239163a..9e5820374328 100644 --- a/drivers/soc/aspeed/aspeed-lpc-snoop.c +++ b/drivers/soc/aspeed/aspeed-lpc-snoop.c @@ -194,28 +194,30 @@ 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 (WARN_ON(lpc_snoop->chan[index].enabled)) + channel = &lpc_snoop->chan[index]; + + if (WARN_ON(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; @@ -246,14 +248,14 @@ 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; err_misc_deregister: - misc_deregister(&lpc_snoop->chan[index].miscdev); + misc_deregister(&channel->miscdev); err_free_fifo: - kfifo_free(&lpc_snoop->chan[index].fifo); + kfifo_free(&channel->fifo); return rc; } @@ -261,7 +263,11 @@ __attribute__((nonnull)) static void aspeed_lpc_disable_snoop(struct aspeed_lpc_snoop *lpc_snoop, enum aspeed_lpc_snoop_index index) { - if (!lpc_snoop->chan[index].enabled) + struct aspeed_lpc_snoop_channel *channel; + + channel = &lpc_snoop->chan[index]; + + if (!channel->enabled) return; /* Disable interrupts along with the device */ @@ -280,10 +286,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) From 08ebd4c56aa23c710fa9d6832ae1df225d35ea0c Mon Sep 17 00:00:00 2001 From: Andrew Jeffery Date: Mon, 16 Jun 2025 22:43:44 +0930 Subject: [PATCH 07/10] soc: aspeed: lpc-snoop: Switch to devm_clk_get_enabled() Simplify clock handling as done in other drivers. Link: https://patch.msgid.link/20250616-aspeed-lpc-snoop-fixes-v2-7-3cdd59c934d3@codeconstruct.com.au Acked-by: Jean Delvare Signed-off-by: Andrew Jeffery --- drivers/soc/aspeed/aspeed-lpc-snoop.c | 20 ++++---------------- 1 file changed, 4 insertions(+), 16 deletions(-) diff --git a/drivers/soc/aspeed/aspeed-lpc-snoop.c b/drivers/soc/aspeed/aspeed-lpc-snoop.c index 9e5820374328..d07c3b2d2940 100644 --- a/drivers/soc/aspeed/aspeed-lpc-snoop.c +++ b/drivers/soc/aspeed/aspeed-lpc-snoop.c @@ -328,26 +328,21 @@ static int aspeed_lpc_snoop_probe(struct platform_device *pdev) return -ENODEV; } - lpc_snoop->clk = devm_clk_get(dev, NULL); + lpc_snoop->clk = devm_clk_get_enabled(dev, NULL); if (IS_ERR(lpc_snoop->clk)) { rc = PTR_ERR(lpc_snoop->clk); if (rc != -EPROBE_DEFER) dev_err(dev, "couldn't get clock\n"); return rc; } - rc = clk_prepare_enable(lpc_snoop->clk); - if (rc) { - dev_err(dev, "couldn't enable clock\n"); - return rc; - } rc = aspeed_lpc_snoop_config_irq(lpc_snoop, pdev); if (rc) - goto err; + return rc; rc = aspeed_lpc_enable_snoop(lpc_snoop, dev, ASPEED_LPC_SNOOP_INDEX_0, port); if (rc) - goto err; + return rc; /* Configuration of 2nd snoop channel port is optional */ if (of_property_read_u32_index(dev->of_node, "snoop-ports", @@ -355,16 +350,11 @@ static int aspeed_lpc_snoop_probe(struct platform_device *pdev) rc = aspeed_lpc_enable_snoop(lpc_snoop, dev, ASPEED_LPC_SNOOP_INDEX_1, port); if (rc) { aspeed_lpc_disable_snoop(lpc_snoop, ASPEED_LPC_SNOOP_INDEX_0); - goto err; + return rc; } } return 0; - -err: - clk_disable_unprepare(lpc_snoop->clk); - - return rc; } static void aspeed_lpc_snoop_remove(struct platform_device *pdev) @@ -374,8 +364,6 @@ static void aspeed_lpc_snoop_remove(struct platform_device *pdev) /* Disable both snoop channels */ 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); } static const struct aspeed_lpc_snoop_model_data ast2400_model_data = { From fa4ffb06d8e4c243ca1073d321068ee8ab384b4b Mon Sep 17 00:00:00 2001 From: Andrew Jeffery Date: Mon, 16 Jun 2025 22:43:45 +0930 Subject: [PATCH 08/10] soc: aspeed: lpc-snoop: Use dev_err_probe() where possible Exploit that it returns the provided error to eliminate some lines, and return the actual error involved rather than -ENODEV. Link: https://patch.msgid.link/20250616-aspeed-lpc-snoop-fixes-v2-8-3cdd59c934d3@codeconstruct.com.au Acked-by: Jean Delvare Signed-off-by: Andrew Jeffery --- drivers/soc/aspeed/aspeed-lpc-snoop.c | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/drivers/soc/aspeed/aspeed-lpc-snoop.c b/drivers/soc/aspeed/aspeed-lpc-snoop.c index d07c3b2d2940..136e30c707ed 100644 --- a/drivers/soc/aspeed/aspeed-lpc-snoop.c +++ b/drivers/soc/aspeed/aspeed-lpc-snoop.c @@ -12,6 +12,7 @@ #include #include +#include #include #include #include @@ -315,10 +316,8 @@ static int aspeed_lpc_snoop_probe(struct platform_device *pdev) } lpc_snoop->regmap = syscon_node_to_regmap(np); - if (IS_ERR(lpc_snoop->regmap)) { - dev_err(dev, "Couldn't get regmap\n"); - return -ENODEV; - } + if (IS_ERR(lpc_snoop->regmap)) + return dev_err_probe(dev, PTR_ERR(lpc_snoop->regmap), "Couldn't get regmap\n"); dev_set_drvdata(&pdev->dev, lpc_snoop); @@ -329,12 +328,8 @@ static int aspeed_lpc_snoop_probe(struct platform_device *pdev) } lpc_snoop->clk = devm_clk_get_enabled(dev, NULL); - if (IS_ERR(lpc_snoop->clk)) { - rc = PTR_ERR(lpc_snoop->clk); - if (rc != -EPROBE_DEFER) - dev_err(dev, "couldn't get clock\n"); - return rc; - } + if (IS_ERR(lpc_snoop->clk)) + return dev_err_probe(dev, PTR_ERR(lpc_snoop->clk), "couldn't get clock"); rc = aspeed_lpc_snoop_config_irq(lpc_snoop, pdev); if (rc) From 4483e3c481bd2d026813434f3cd93381386cd1fa Mon Sep 17 00:00:00 2001 From: Andrew Jeffery Date: Mon, 16 Jun 2025 22:43:46 +0930 Subject: [PATCH 09/10] soc: aspeed: lpc-snoop: Consolidate channel initialisation Previously, channel initialisation was a bit perilous with respect to resource cleanup in error paths. While the implementation had issues, it at least made an effort to eliminate some of its problems by first testing whether any channels were enabled, and bailing out if not. Having improved the robustness of resource handling in probe() we can now rearrange the initial channel test to be located with the subsequent test, and rework the unrolled conditional logic to use a loop for an improvement in readability. Link: https://patch.msgid.link/20250616-aspeed-lpc-snoop-fixes-v2-9-3cdd59c934d3@codeconstruct.com.au Acked-by: Jean Delvare Signed-off-by: Andrew Jeffery --- drivers/soc/aspeed/aspeed-lpc-snoop.c | 51 +++++++++++++-------------- 1 file changed, 24 insertions(+), 27 deletions(-) diff --git a/drivers/soc/aspeed/aspeed-lpc-snoop.c b/drivers/soc/aspeed/aspeed-lpc-snoop.c index 136e30c707ed..4c2b458f4684 100644 --- a/drivers/soc/aspeed/aspeed-lpc-snoop.c +++ b/drivers/soc/aspeed/aspeed-lpc-snoop.c @@ -293,12 +293,21 @@ static void aspeed_lpc_disable_snoop(struct aspeed_lpc_snoop *lpc_snoop, kfifo_free(&channel->fifo); } +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, ASPEED_LPC_SNOOP_INDEX_0); + aspeed_lpc_disable_snoop(lpc_snoop, ASPEED_LPC_SNOOP_INDEX_1); +} + static int aspeed_lpc_snoop_probe(struct platform_device *pdev) { struct aspeed_lpc_snoop *lpc_snoop; - struct device *dev; struct device_node *np; - u32 port; + struct device *dev; + int idx; int rc; dev = &pdev->dev; @@ -321,12 +330,6 @@ static int aspeed_lpc_snoop_probe(struct platform_device *pdev) dev_set_drvdata(&pdev->dev, lpc_snoop); - rc = of_property_read_u32_index(dev->of_node, "snoop-ports", 0, &port); - if (rc) { - dev_err(dev, "no snoop ports configured\n"); - return -ENODEV; - } - lpc_snoop->clk = devm_clk_get_enabled(dev, NULL); if (IS_ERR(lpc_snoop->clk)) return dev_err_probe(dev, PTR_ERR(lpc_snoop->clk), "couldn't get clock"); @@ -335,30 +338,24 @@ static int aspeed_lpc_snoop_probe(struct platform_device *pdev) if (rc) return rc; - rc = aspeed_lpc_enable_snoop(lpc_snoop, dev, ASPEED_LPC_SNOOP_INDEX_0, port); - if (rc) - return rc; + for (idx = ASPEED_LPC_SNOOP_INDEX_0; idx <= ASPEED_LPC_SNOOP_INDEX_MAX; idx++) { + u32 port; - /* 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, ASPEED_LPC_SNOOP_INDEX_1, port); - if (rc) { - aspeed_lpc_disable_snoop(lpc_snoop, ASPEED_LPC_SNOOP_INDEX_0); - return rc; - } + rc = of_property_read_u32_index(dev->of_node, "snoop-ports", idx, &port); + if (rc) + break; + + rc = aspeed_lpc_enable_snoop(lpc_snoop, dev, idx, port); + if (rc) + goto cleanup_channels; } - return 0; -} + return idx == ASPEED_LPC_SNOOP_INDEX_0 ? -ENODEV : 0; -static void aspeed_lpc_snoop_remove(struct platform_device *pdev) -{ - struct aspeed_lpc_snoop *lpc_snoop = dev_get_drvdata(&pdev->dev); +cleanup_channels: + aspeed_lpc_snoop_remove(pdev); - /* Disable both snoop channels */ - aspeed_lpc_disable_snoop(lpc_snoop, ASPEED_LPC_SNOOP_INDEX_0); - aspeed_lpc_disable_snoop(lpc_snoop, ASPEED_LPC_SNOOP_INDEX_1); + return rc; } static const struct aspeed_lpc_snoop_model_data ast2400_model_data = { From fdf003f30b99e232cd3e61cc42d836ed14d08ccb Mon Sep 17 00:00:00 2001 From: Andrew Jeffery Date: Mon, 16 Jun 2025 22:43:47 +0930 Subject: [PATCH 10/10] soc: aspeed: lpc-snoop: Lift channel config to const structs 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. Link: https://patch.msgid.link/20250616-aspeed-lpc-snoop-fixes-v2-10-3cdd59c934d3@codeconstruct.com.au Signed-off-by: Andrew Jeffery --- drivers/soc/aspeed/aspeed-lpc-snoop.c | 99 ++++++++++++--------------- 1 file changed, 45 insertions(+), 54 deletions(-) diff --git a/drivers/soc/aspeed/aspeed-lpc-snoop.c b/drivers/soc/aspeed/aspeed-lpc-snoop.c index 4c2b458f4684..b03310c0830d 100644 --- a/drivers/soc/aspeed/aspeed-lpc-snoop.c +++ b/drivers/soc/aspeed/aspeed-lpc-snoop.c @@ -63,7 +63,16 @@ enum aspeed_lpc_snoop_index { ASPEED_LPC_SNOOP_INDEX_MAX = ASPEED_LPC_SNOOP_INDEX_1, }; +struct aspeed_lpc_snoop_channel_cfg { + enum aspeed_lpc_snoop_index index; + 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; @@ -77,6 +86,23 @@ struct aspeed_lpc_snoop { struct aspeed_lpc_snoop_channel chan[ASPEED_LPC_SNOOP_INDEX_MAX + 1]; }; +static const struct aspeed_lpc_snoop_channel_cfg channel_cfgs[ASPEED_LPC_SNOOP_INDEX_MAX + 1] = { + { + .index = ASPEED_LPC_SNOOP_INDEX_0, + .hicr5_en = HICR5_EN_SNP0W | HICR5_ENINT_SNP0W, + .snpwadr_mask = SNPWADR_CH0_MASK, + .snpwadr_shift = SNPWADR_CH0_SHIFT, + .hicrb_en = HICRB_ENSNP0D, + }, + { + .index = ASPEED_LPC_SNOOP_INDEX_1, + .hicr5_en = HICR5_EN_SNP1W | HICR5_ENINT_SNP1W, + .snpwadr_mask = SNPWADR_CH1_MASK, + .snpwadr_shift = SNPWADR_CH1_SHIFT, + .hicrb_en = HICRB_ENSNP1D, + }, +}; + static struct aspeed_lpc_snoop_channel *snoop_file_to_chan(struct file *file) { return container_of(file->private_data, @@ -189,28 +215,27 @@ 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, - enum aspeed_lpc_snoop_index index, u16 lpc_port) +static int aspeed_lpc_enable_snoop(struct device *dev, + struct aspeed_lpc_snoop *lpc_snoop, + struct aspeed_lpc_snoop_channel *channel, + const struct aspeed_lpc_snoop_channel_cfg *cfg, + 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; - channel = &lpc_snoop->chan[index]; - if (WARN_ON(channel->enabled)) return -EBUSY; init_waitqueue_head(&channel->wq); + channel->cfg = cfg; 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); + devm_kasprintf(dev, GFP_KERNEL, "%s%d", DEVICE_NAME, cfg->index); if (!channel->miscdev.name) return -ENOMEM; @@ -223,38 +248,18 @@ 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; - } - - 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_set_bits(lpc_snoop->regmap, HICR5, cfg->hicr5_en); + regmap_update_bits(lpc_snoop->regmap, SNPWADR, cfg->snpwadr_mask, + lpc_port << 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_set_bits(lpc_snoop->regmap, HICRB, cfg->hicrb_en); channel->enabled = true; return 0; -err_misc_deregister: - misc_deregister(&channel->miscdev); err_free_fifo: kfifo_free(&channel->fifo); return rc; @@ -262,30 +267,13 @@ static int aspeed_lpc_enable_snoop(struct aspeed_lpc_snoop *lpc_snoop, __attribute__((nonnull)) static void aspeed_lpc_disable_snoop(struct aspeed_lpc_snoop *lpc_snoop, - enum aspeed_lpc_snoop_index index) + struct aspeed_lpc_snoop_channel *channel) { - struct aspeed_lpc_snoop_channel *channel; - - channel = &lpc_snoop->chan[index]; - 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_clear_bits(lpc_snoop->regmap, HICR5, channel->cfg->hicr5_en); channel->enabled = false; /* Consider improving safety wrt concurrent reader(s) */ @@ -298,8 +286,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, ASPEED_LPC_SNOOP_INDEX_0); - aspeed_lpc_disable_snoop(lpc_snoop, ASPEED_LPC_SNOOP_INDEX_1); + aspeed_lpc_disable_snoop(lpc_snoop, &lpc_snoop->chan[0]); + aspeed_lpc_disable_snoop(lpc_snoop, &lpc_snoop->chan[1]); } static int aspeed_lpc_snoop_probe(struct platform_device *pdev) @@ -338,6 +326,8 @@ static int aspeed_lpc_snoop_probe(struct platform_device *pdev) if (rc) return rc; + static_assert(ARRAY_SIZE(channel_cfgs) == ARRAY_SIZE(lpc_snoop->chan), + "Broken implementation assumption regarding cfg count"); for (idx = ASPEED_LPC_SNOOP_INDEX_0; idx <= ASPEED_LPC_SNOOP_INDEX_MAX; idx++) { u32 port; @@ -345,7 +335,8 @@ static int aspeed_lpc_snoop_probe(struct platform_device *pdev) if (rc) break; - rc = aspeed_lpc_enable_snoop(lpc_snoop, dev, idx, port); + rc = aspeed_lpc_enable_snoop(dev, lpc_snoop, &lpc_snoop->chan[idx], + &channel_cfgs[idx], port); if (rc) goto cleanup_channels; }