[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

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 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