Linux-ARM-Kernel Archive on lore.kernel.org
 help / color / mirror / Atom feed
* Re: [PATCH v2 0/3] drm/mediatek: Add support for OF graphs
From: Alexandre Mergnat @ 2024-04-30 10:17 UTC (permalink / raw)
  To: AngeloGioacchino Del Regno, chunkuang.hu
  Cc: robh, krzysztof.kozlowski+dt, conor+dt, p.zabel, airlied, daniel,
	maarten.lankhorst, mripard, tzimmermann, matthias.bgg, shawn.sung,
	yu-chang.lee, ck.hu, jitao.shi, devicetree, linux-kernel,
	dri-devel, linux-mediatek, linux-arm-kernel, wenst, kernel
In-Reply-To: <20240409120211.321153-1-angelogioacchino.delregno@collabora.com>

Hi Angelo,

On 09/04/2024 14:02, AngeloGioacchino Del Regno wrote:
> This series was tested on MT8195 Cherry Tomato and on MT8395 Radxa
> NIO-12L with both hardcoded paths, OF graph support and partially
> hardcoded paths (meaning main display through OF graph and external
> display hardcoded, because of OVL_ADAPTOR).

Is that make sense for you to add the DTS changes of these boards into this serie ?
I asked because, IMHO, that could help to understand the serie.

-- 
Regards,
Alexandre

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* Re: [PATCH v8 04/16] ACPI: processor: Move checks and availability of acpi_processor earlier
From: Rafael J. Wysocki @ 2024-04-30 10:17 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: Gavin Shan, Thomas Gleixner, Peter Zijlstra, linux-pm, loongarch,
	linux-acpi, linux-arch, linux-kernel, linux-arm-kernel, kvmarm,
	x86, Russell King, Rafael J . Wysocki, Miguel Luis, James Morse,
	Salil Mehta, Jean-Philippe Brucker, Catalin Marinas, Will Deacon,
	Marc Zyngier, Hanjun Guo, Ingo Molnar, Borislav Petkov,
	Dave Hansen, linuxarm, justin.he, jianyong.wu, Lorenzo Pieralisi,
	Sudeep Holla
In-Reply-To: <20240430111341.00003dba@huawei.com>

On Tue, Apr 30, 2024 at 12:13 PM Jonathan Cameron
<Jonathan.Cameron@huawei.com> wrote:
>
> On Tue, 30 Apr 2024 10:28:38 +0100
> Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:
>
> > On Tue, 30 Apr 2024 14:17:24 +1000
> > Gavin Shan <gshan@redhat.com> wrote:
> >
> > > On 4/26/24 23:51, Jonathan Cameron wrote:
> > > > Make the per_cpu(processors, cpu) entries available earlier so that
> > > > they are available in arch_register_cpu() as ARM64 will need access
> > > > to the acpi_handle to distinguish between acpi_processor_add()
> > > > and earlier registration attempts (which will fail as _STA cannot
> > > > be checked).
> > > >
> > > > Reorder the remove flow to clear this per_cpu() after
> > > > arch_unregister_cpu() has completed, allowing it to be used in
> > > > there as well.
> > > >
> > > > Note that on x86 for the CPU hotplug case, the pr->id prior to
> > > > acpi_map_cpu() may be invalid. Thus the per_cpu() structures
> > > > must be initialized after that call or after checking the ID
> > > > is valid (not hotplug path).
> > > >
> > > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > >
> > > > ---
> > > > v8: On buggy bios detection when setting per_cpu structures
> > > >      do not carry on.
> > > >      Fix up the clearing of per cpu structures to remove unwanted
> > > >      side effects and ensure an error code isn't use to reference them.
> > > > ---
> > > >   drivers/acpi/acpi_processor.c | 79 +++++++++++++++++++++--------------
> > > >   1 file changed, 48 insertions(+), 31 deletions(-)
> > > >
> > > > diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c
> > > > index ba0a6f0ac841..3b180e21f325 100644
> > > > --- a/drivers/acpi/acpi_processor.c
> > > > +++ b/drivers/acpi/acpi_processor.c
> > > > @@ -183,8 +183,38 @@ static void __init acpi_pcc_cpufreq_init(void) {}
> > > >   #endif /* CONFIG_X86 */
> > > >
> > > >   /* Initialization */
> > > > +static DEFINE_PER_CPU(void *, processor_device_array);
> > > > +
> > > > +static bool acpi_processor_set_per_cpu(struct acpi_processor *pr,
> > > > +                                struct acpi_device *device)
> > > > +{
> > > > + BUG_ON(pr->id >= nr_cpu_ids);
> > >
> > > One blank line after BUG_ON() if we need to follow original implementation.
> >
> > Sure unintentional - I'll put that back.
> >
> > >
> > > > + /*
> > > > +  * Buggy BIOS check.
> > > > +  * ACPI id of processors can be reported wrongly by the BIOS.
> > > > +  * Don't trust it blindly
> > > > +  */
> > > > + if (per_cpu(processor_device_array, pr->id) != NULL &&
> > > > +     per_cpu(processor_device_array, pr->id) != device) {
> > > > +         dev_warn(&device->dev,
> > > > +                  "BIOS reported wrong ACPI id %d for the processor\n",
> > > > +                  pr->id);
> > > > +         /* Give up, but do not abort the namespace scan. */
> > >
> > > It depends on how the return value is handled by the caller if the namespace
> > > is continued to be scanned. The caller can be acpi_processor_hotadd_init()
> > > and acpi_processor_get_info() after this patch is applied. So I think this
> > > specific comment need to be moved to the caller.
> >
> > Good point. This gets messy and was an unintended change.
> >
> > Previously the options were:
> > 1) acpi_processor_get_info() failed for other reasons - this code was never called.
> > 2) acpi_processor_get_info() succeeded without acpi_processor_hotadd_init (non hotplug)
> >    this code then ran and would paper over the problem doing a bunch of cleanup under err.
> > 3) acpi_processor_get_info() succeeded with acpi_processor_hotadd_init called.
> >    This code then ran and would paper over the problem doing a bunch of cleanup under err.
> >
> > We should maintain that or argue cleanly against it.
> >
> > This isn't helped the the fact I have no idea which cases we care about for that bios
> > bug handling.  Do any of those bios's ever do hotplug?  Guess we have to try and maintain
> > whatever protection this was offering.
> >
> > Also, the original code leaks data in some paths and I have limited idea
> > of whether it is intentional or not. So to tidy the issue up that you've identified
> > I'll need to try and make that code consistent first.
> >
> > I suspect the only way to do that is going to be to duplicate the allocations we
> > 'want' to leak to deal with the bios bug detection.
> >
> > For example acpi_processor_get_info() failing leaks pr and pr->throttling.shared_cpu_map
> > before this series. After this series we need pr to leak because it's used for the detection
> > via processor_device_array.
> >
> > I'll work through this but it's going to be tricky to tell if we get right.
> > Step 1 will be closing the existing leaks and then we will have something
> > consistent to build on.
> >
> I 'think' that fixing the original leaks makes this all much more straight forward.
> That return 0 for acpi_processor_get_info() never made sense as far as I can tell.
> The pr isn't used after this point.
>
> What about something along lines of.
>
> diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c
> index 161c95c9d60a..97cff4492304 100644
> --- a/drivers/acpi/acpi_processor.c
> +++ b/drivers/acpi/acpi_processor.c
> @@ -392,8 +392,10 @@ static int acpi_processor_add(struct acpi_device *device,
>         device->driver_data = pr;
>
>         result = acpi_processor_get_info(device);
> -       if (result) /* Processor is not physically present or unavailable */
> -               return 0;
> +       if (result) { /* Processor is not physically present or unavailable */
> +               result = 0;

As per my previous message (just sent) this should be an error code,
as returning 0 from acpi_processor_add() is generally problematic.

> +               goto err_free_throttling_mask;
> +       }
>
>         BUG_ON(pr->id >= nr_cpu_ids);
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* Re: [PATCH v8 04/16] ACPI: processor: Move checks and availability of acpi_processor earlier
From: Jonathan Cameron @ 2024-04-30 10:13 UTC (permalink / raw)
  To: Gavin Shan
  Cc: Thomas Gleixner, Peter Zijlstra, linux-pm, loongarch, linux-acpi,
	linux-arch, linux-kernel, linux-arm-kernel, kvmarm, x86,
	Russell King, Rafael J . Wysocki, Miguel Luis, James Morse,
	Salil Mehta, Jean-Philippe Brucker, Catalin Marinas, Will Deacon,
	Marc Zyngier, Hanjun Guo, Ingo Molnar, Borislav Petkov,
	Dave Hansen, linuxarm, justin.he, jianyong.wu, Lorenzo Pieralisi,
	Sudeep Holla
In-Reply-To: <20240430102838.00006e04@Huawei.com>

On Tue, 30 Apr 2024 10:28:38 +0100
Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:

> On Tue, 30 Apr 2024 14:17:24 +1000
> Gavin Shan <gshan@redhat.com> wrote:
> 
> > On 4/26/24 23:51, Jonathan Cameron wrote:  
> > > Make the per_cpu(processors, cpu) entries available earlier so that
> > > they are available in arch_register_cpu() as ARM64 will need access
> > > to the acpi_handle to distinguish between acpi_processor_add()
> > > and earlier registration attempts (which will fail as _STA cannot
> > > be checked).
> > > 
> > > Reorder the remove flow to clear this per_cpu() after
> > > arch_unregister_cpu() has completed, allowing it to be used in
> > > there as well.
> > > 
> > > Note that on x86 for the CPU hotplug case, the pr->id prior to
> > > acpi_map_cpu() may be invalid. Thus the per_cpu() structures
> > > must be initialized after that call or after checking the ID
> > > is valid (not hotplug path).
> > > 
> > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > 
> > > ---
> > > v8: On buggy bios detection when setting per_cpu structures
> > >      do not carry on.
> > >      Fix up the clearing of per cpu structures to remove unwanted
> > >      side effects and ensure an error code isn't use to reference them.
> > > ---
> > >   drivers/acpi/acpi_processor.c | 79 +++++++++++++++++++++--------------
> > >   1 file changed, 48 insertions(+), 31 deletions(-)
> > > 
> > > diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c
> > > index ba0a6f0ac841..3b180e21f325 100644
> > > --- a/drivers/acpi/acpi_processor.c
> > > +++ b/drivers/acpi/acpi_processor.c
> > > @@ -183,8 +183,38 @@ static void __init acpi_pcc_cpufreq_init(void) {}
> > >   #endif /* CONFIG_X86 */
> > >   
> > >   /* Initialization */
> > > +static DEFINE_PER_CPU(void *, processor_device_array);
> > > +
> > > +static bool acpi_processor_set_per_cpu(struct acpi_processor *pr,
> > > +				       struct acpi_device *device)
> > > +{
> > > +	BUG_ON(pr->id >= nr_cpu_ids);    
> > 
> > One blank line after BUG_ON() if we need to follow original implementation.  
> 
> Sure unintentional - I'll put that back.
> 
> >   
> > > +	/*
> > > +	 * Buggy BIOS check.
> > > +	 * ACPI id of processors can be reported wrongly by the BIOS.
> > > +	 * Don't trust it blindly
> > > +	 */
> > > +	if (per_cpu(processor_device_array, pr->id) != NULL &&
> > > +	    per_cpu(processor_device_array, pr->id) != device) {
> > > +		dev_warn(&device->dev,
> > > +			 "BIOS reported wrong ACPI id %d for the processor\n",
> > > +			 pr->id);
> > > +		/* Give up, but do not abort the namespace scan. */    
> > 
> > It depends on how the return value is handled by the caller if the namespace
> > is continued to be scanned. The caller can be acpi_processor_hotadd_init()
> > and acpi_processor_get_info() after this patch is applied. So I think this
> > specific comment need to be moved to the caller.  
> 
> Good point. This gets messy and was an unintended change.
> 
> Previously the options were:
> 1) acpi_processor_get_info() failed for other reasons - this code was never called.
> 2) acpi_processor_get_info() succeeded without acpi_processor_hotadd_init (non hotplug)
>    this code then ran and would paper over the problem doing a bunch of cleanup under err.
> 3) acpi_processor_get_info() succeeded with acpi_processor_hotadd_init called.
>    This code then ran and would paper over the problem doing a bunch of cleanup under err.
> 
> We should maintain that or argue cleanly against it.
> 
> This isn't helped the the fact I have no idea which cases we care about for that bios
> bug handling.  Do any of those bios's ever do hotplug?  Guess we have to try and maintain
> whatever protection this was offering.
> 
> Also, the original code leaks data in some paths and I have limited idea
> of whether it is intentional or not. So to tidy the issue up that you've identified
> I'll need to try and make that code consistent first.
> 
> I suspect the only way to do that is going to be to duplicate the allocations we
> 'want' to leak to deal with the bios bug detection.
> 
> For example acpi_processor_get_info() failing leaks pr and pr->throttling.shared_cpu_map
> before this series. After this series we need pr to leak because it's used for the detection
> via processor_device_array.
> 
> I'll work through this but it's going to be tricky to tell if we get right.
> Step 1 will be closing the existing leaks and then we will have something
> consistent to build on.
> 
I 'think' that fixing the original leaks makes this all much more straight forward.
That return 0 for acpi_processor_get_info() never made sense as far as I can tell.
The pr isn't used after this point.

What about something along lines of.

diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c
index 161c95c9d60a..97cff4492304 100644
--- a/drivers/acpi/acpi_processor.c
+++ b/drivers/acpi/acpi_processor.c
@@ -392,8 +392,10 @@ static int acpi_processor_add(struct acpi_device *device,
        device->driver_data = pr;

        result = acpi_processor_get_info(device);
-       if (result) /* Processor is not physically present or unavailable */
-               return 0;
+       if (result) { /* Processor is not physically present or unavailable */
+               result = 0;
+               goto err_free_throttling_mask;
+       }

        BUG_ON(pr->id >= nr_cpu_ids);

@@ -408,7 +410,7 @@ static int acpi_processor_add(struct acpi_device *device,
                        "BIOS reported wrong ACPI id %d for the processor\n",
                        pr->id);
                /* Give up, but do not abort the namespace scan. */
-               goto err;
+               goto err_clear_driver_data;
        }
        /*
         * processor_device_array is not cleared on errors to allow buggy BIOS
@@ -420,12 +422,12 @@ static int acpi_processor_add(struct acpi_device *device,
        dev = get_cpu_device(pr->id);
        if (!dev) {
                result = -ENODEV;
-               goto err;
+               goto err_clear_per_cpu;
        }

        result = acpi_bind_one(dev, device);
        if (result)
-               goto err;
+               goto err_clear_per_cpu;

        pr->dev = dev;

@@ -436,10 +438,12 @@ static int acpi_processor_add(struct acpi_device *device,
        dev_err(dev, "Processor driver could not be attached\n");
        acpi_unbind_one(dev);

- err:
-       free_cpumask_var(pr->throttling.shared_cpu_map);
-       device->driver_data = NULL;
+ err_clear_per_cpu:
        per_cpu(processors, pr->id) = NULL;
+ err_clear_driver_data:
+       device->driver_data = NULL;
+ err_free_throttling_mask:
+       free_cpumask_var(pr->throttling.shared_cpu_map);
  err_free_pr:
        kfree(pr);
        return result;

Then the diff on this patch is simply:

diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c
index 3c49eae1e943..3b75f5aeb7ab 100644
--- a/drivers/acpi/acpi_processor.c
+++ b/drivers/acpi/acpi_processor.c
@@ -200,7 +200,6 @@ static bool acpi_processor_set_per_cpu(struct acpi_processor *pr,
                dev_warn(&device->dev,
                         "BIOS reported wrong ACPI id %d for the processor\n",
                         pr->id);
-               /* Give up, but do not abort the namespace scan. */
                return false;
        }
        /*
@@ -230,13 +229,14 @@ static int acpi_processor_hotadd_init(struct acpi_processor *pr,
                goto out;

        if (!acpi_processor_set_per_cpu(pr, device)) {
+               ret = -EINVAL;
                acpi_unmap_cpu(pr->id);
                goto out;
        }

        ret = arch_register_cpu(pr->id);
        if (ret) {
-               /* Leave the processor device array in place to detect buggy bios */
+x              /* Leave the processor device array in place to detect buggy bios */
                per_cpu(processors, pr->id) = NULL;
                acpi_unmap_cpu(pr->id);
                goto out;
@@ -262,7 +262,7 @@ static inline int acpi_processor_hotadd_init(struct acpi_processor *pr,
 }
 #endif /* CONFIG_ACPI_HOTPLUG_CPU */

-static int acpi_processor_get_info(struct acpi_device *device)
+static int acpi_processor_get_info(struct acpi_device *device, bool bios_bug)
 {
        union acpi_object object = { 0 };
        struct acpi_buffer buffer = { sizeof(union acpi_object), &object };
@@ -361,7 +361,7 @@ static int acpi_processor_get_info(struct acpi_device *device)
                        return ret;
        } else {
                if (!acpi_processor_set_per_cpu(pr, device))
-                       return 0;
+                       return -EINVAL;
        }

        /*
> > 
> > Besides, it seems acpi_processor_set_per_cpu() isn't properly called and
> > memory leakage can happen. More details are given below.
> >   
> > > +		return false;
> > > +	}
> > > +	/*
> > > +	 * processor_device_array is not cleared on errors to allow buggy BIOS
> > > +	 * checks.
> > > +	 */
> > > +	per_cpu(processor_device_array, pr->id) = device;
> > > +	per_cpu(processors, pr->id) = pr;
> > > +
> > > +	return true;
> > > +}
> > > +
> > >   #ifdef CONFIG_ACPI_HOTPLUG_CPU
> > > -static int acpi_processor_hotadd_init(struct acpi_processor *pr)
> > > +static int acpi_processor_hotadd_init(struct acpi_processor *pr,
> > > +				      struct acpi_device *device)
> > >   {
> > >   	int ret;
> > >   
> > > @@ -198,8 +228,15 @@ static int acpi_processor_hotadd_init(struct acpi_processor *pr)
> > >   	if (ret)
> > >   		goto out;
> > >   
> > > +	if (!acpi_processor_set_per_cpu(pr, device)) {
> > > +		acpi_unmap_cpu(pr->id);
> > > +		goto out;
> > > +	}
> > > +    
> > 
> > With the 'goto out', zero is returned from acpi_processor_hotadd_init() to acpi_processor_get_info().

Indeed a bug :(

> > The zero return value is carried from acpi_map_cpu() in acpi_processor_hotadd_init(). If I'm correct,
> > we need return errno from acpi_processor_get_info() to acpi_processor_add() so that cleanup can be
> > done. For example, the cleanup corresponding to the 'err' tag can be done in acpi_processor_add().
> > Otherwise, we will have memory leakage.

The confusion here was that previously acpi_processor_add() was missing error cleanup for
acpi_processor_get_info().  With that in place I think it's all much simpler.

Thanks for your eagle eyes!

Jonathan


> >   
> > >   	ret = arch_register_cpu(pr->id);
> > >   	if (ret) {
> > > +		/* Leave the processor device array in place to detect buggy bios */
> > > +		per_cpu(processors, pr->id) = NULL;
> > >   		acpi_unmap_cpu(pr->id);
> > >   		goto out;
> > >   	}
> > > @@ -217,7 +254,8 @@ static int acpi_processor_hotadd_init(struct acpi_processor *pr)
> > >   	return ret;
> > >   }
> > >   #else
> > > -static inline int acpi_processor_hotadd_init(struct acpi_processor *pr)
> > > +static inline int acpi_processor_hotadd_init(struct acpi_processor *pr,
> > > +					     struct acpi_device *device)
> > >   {
> > >   	return -ENODEV;
> > >   }
> > > @@ -316,10 +354,13 @@ static int acpi_processor_get_info(struct acpi_device *device)
> > >   	 *  because cpuid <-> apicid mapping is persistent now.
> > >   	 */
> > >   	if (invalid_logical_cpuid(pr->id) || !cpu_present(pr->id)) {
> > > -		int ret = acpi_processor_hotadd_init(pr);
> > > +		int ret = acpi_processor_hotadd_init(pr, device);
> > >   
> > >   		if (ret)
> > >   			return ret;
> > > +	} else {
> > > +		if (!acpi_processor_set_per_cpu(pr, device))
> > > +			return 0;
> > >   	}
> > >       
> > 
> > For non-hotplug case, we still need pass the error to acpi_processor_add() so that
> > cleanup corresponding 'err' tag can be done. Otherwise, we will have memory leakage.
> >   
> > >   	/*
> > > @@ -365,8 +406,6 @@ static int acpi_processor_get_info(struct acpi_device *device)
> > >    * (cpu_data(cpu)) values, like CPU feature flags, family, model, etc.
> > >    * Such things have to be put in and set up by the processor driver's .probe().
> > >    */
> > > -static DEFINE_PER_CPU(void *, processor_device_array);
> > > -
> > >   static int acpi_processor_add(struct acpi_device *device,
> > >   					const struct acpi_device_id *id)
> > >   {
> > > @@ -395,28 +434,6 @@ static int acpi_processor_add(struct acpi_device *device,
> > >   	if (result) /* Processor is not physically present or unavailable */
> > >   		return 0;
> > >   
> > > -	BUG_ON(pr->id >= nr_cpu_ids);
> > > -
> > > -	/*
> > > -	 * Buggy BIOS check.
> > > -	 * ACPI id of processors can be reported wrongly by the BIOS.
> > > -	 * Don't trust it blindly
> > > -	 */
> > > -	if (per_cpu(processor_device_array, pr->id) != NULL &&
> > > -	    per_cpu(processor_device_array, pr->id) != device) {
> > > -		dev_warn(&device->dev,
> > > -			"BIOS reported wrong ACPI id %d for the processor\n",
> > > -			pr->id);
> > > -		/* Give up, but do not abort the namespace scan. */
> > > -		goto err;
> > > -	}
> > > -	/*
> > > -	 * processor_device_array is not cleared on errors to allow buggy BIOS
> > > -	 * checks.
> > > -	 */
> > > -	per_cpu(processor_device_array, pr->id) = device;
> > > -	per_cpu(processors, pr->id) = pr;
> > > -
> > >   	dev = get_cpu_device(pr->id);
> > >   	if (!dev) {
> > >   		result = -ENODEV;
> > > @@ -469,10 +486,6 @@ static void acpi_processor_remove(struct acpi_device *device)
> > >   	device_release_driver(pr->dev);
> > >   	acpi_unbind_one(pr->dev);
> > >   
> > > -	/* Clean up. */
> > > -	per_cpu(processor_device_array, pr->id) = NULL;
> > > -	per_cpu(processors, pr->id) = NULL;
> > > -
> > >   	cpu_maps_update_begin();
> > >   	cpus_write_lock();
> > >   
> > > @@ -480,6 +493,10 @@ static void acpi_processor_remove(struct acpi_device *device)
> > >   	arch_unregister_cpu(pr->id);
> > >   	acpi_unmap_cpu(pr->id);
> > >   
> > > +	/* Clean up. */
> > > +	per_cpu(processor_device_array, pr->id) = NULL;
> > > +	per_cpu(processors, pr->id) = NULL;
> > > +
> > >   	cpus_write_unlock();
> > >   	cpu_maps_update_done();
> > >       
> > 
> > Thanks,
> > Gavin
> >   
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related

* Re: [PATCH v8 04/16] ACPI: processor: Move checks and availability of acpi_processor earlier
From: Rafael J. Wysocki @ 2024-04-30 10:12 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: Gavin Shan, Thomas Gleixner, Peter Zijlstra, linux-pm, loongarch,
	linux-acpi, linux-arch, linux-kernel, linux-arm-kernel, kvmarm,
	x86, Russell King, Rafael J . Wysocki, Miguel Luis, James Morse,
	Salil Mehta, Jean-Philippe Brucker, Catalin Marinas, Will Deacon,
	Marc Zyngier, Hanjun Guo, Ingo Molnar, Borislav Petkov,
	Dave Hansen, linuxarm, justin.he, jianyong.wu, Lorenzo Pieralisi,
	Sudeep Holla
In-Reply-To: <20240430102838.00006e04@Huawei.com>

On Tue, Apr 30, 2024 at 11:28 AM Jonathan Cameron
<Jonathan.Cameron@huawei.com> wrote:
>
> On Tue, 30 Apr 2024 14:17:24 +1000
> Gavin Shan <gshan@redhat.com> wrote:
>
> > On 4/26/24 23:51, Jonathan Cameron wrote:
> > > Make the per_cpu(processors, cpu) entries available earlier so that
> > > they are available in arch_register_cpu() as ARM64 will need access
> > > to the acpi_handle to distinguish between acpi_processor_add()
> > > and earlier registration attempts (which will fail as _STA cannot
> > > be checked).
> > >
> > > Reorder the remove flow to clear this per_cpu() after
> > > arch_unregister_cpu() has completed, allowing it to be used in
> > > there as well.
> > >
> > > Note that on x86 for the CPU hotplug case, the pr->id prior to
> > > acpi_map_cpu() may be invalid. Thus the per_cpu() structures
> > > must be initialized after that call or after checking the ID
> > > is valid (not hotplug path).
> > >
> > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > >
> > > ---
> > > v8: On buggy bios detection when setting per_cpu structures
> > >      do not carry on.
> > >      Fix up the clearing of per cpu structures to remove unwanted
> > >      side effects and ensure an error code isn't use to reference them.
> > > ---
> > >   drivers/acpi/acpi_processor.c | 79 +++++++++++++++++++++--------------
> > >   1 file changed, 48 insertions(+), 31 deletions(-)
> > >
> > > diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c
> > > index ba0a6f0ac841..3b180e21f325 100644
> > > --- a/drivers/acpi/acpi_processor.c
> > > +++ b/drivers/acpi/acpi_processor.c
> > > @@ -183,8 +183,38 @@ static void __init acpi_pcc_cpufreq_init(void) {}
> > >   #endif /* CONFIG_X86 */
> > >
> > >   /* Initialization */
> > > +static DEFINE_PER_CPU(void *, processor_device_array);
> > > +
> > > +static bool acpi_processor_set_per_cpu(struct acpi_processor *pr,
> > > +                                  struct acpi_device *device)
> > > +{
> > > +   BUG_ON(pr->id >= nr_cpu_ids);
> >
> > One blank line after BUG_ON() if we need to follow original implementation.
>
> Sure unintentional - I'll put that back.
>
> >
> > > +   /*
> > > +    * Buggy BIOS check.
> > > +    * ACPI id of processors can be reported wrongly by the BIOS.
> > > +    * Don't trust it blindly
> > > +    */
> > > +   if (per_cpu(processor_device_array, pr->id) != NULL &&
> > > +       per_cpu(processor_device_array, pr->id) != device) {
> > > +           dev_warn(&device->dev,
> > > +                    "BIOS reported wrong ACPI id %d for the processor\n",
> > > +                    pr->id);
> > > +           /* Give up, but do not abort the namespace scan. */
> >
> > It depends on how the return value is handled by the caller if the namespace
> > is continued to be scanned. The caller can be acpi_processor_hotadd_init()
> > and acpi_processor_get_info() after this patch is applied. So I think this
> > specific comment need to be moved to the caller.
>
> Good point. This gets messy and was an unintended change.
>
> Previously the options were:
> 1) acpi_processor_get_info() failed for other reasons - this code was never called.
> 2) acpi_processor_get_info() succeeded without acpi_processor_hotadd_init (non hotplug)
>    this code then ran and would paper over the problem doing a bunch of cleanup under err.
> 3) acpi_processor_get_info() succeeded with acpi_processor_hotadd_init called.
>    This code then ran and would paper over the problem doing a bunch of cleanup under err.
>
> We should maintain that or argue cleanly against it.

The return value needs to be propagated to acpi_processor_add() so it
can decide what to do with it.

Now, acpi_processor_add() can only return 1 if the CPU has been
successfully registered and initialized, so it is regarded as
available (but it may not be online to start with).

Returning 0 from it may get messy, because acpi_default_enumeration()
will get called and it will attempt to create a platform device for
the CPU, so in all cases in which the CPU is not regarded as available
when acpi_processor_add() returns, it should return an error code (the
exact value doesn't matter for its caller so long as it is negative).

> This isn't helped the the fact I have no idea which cases we care about for that bios
> bug handling.  Do any of those bios's ever do hotplug?  Guess we have to try and maintain
> whatever protection this was offering.
>
> Also, the original code leaks data in some paths and I have limited idea
> of whether it is intentional or not. So to tidy the issue up that you've identified
> I'll need to try and make that code consistent first.

I agree.

> I suspect the only way to do that is going to be to duplicate the allocations we
> 'want' to leak to deal with the bios bug detection.
>
> For example acpi_processor_get_info() failing leaks pr and pr->throttling.shared_cpu_map
> before this series. After this series we need pr to leak because it's used for the detection
> via processor_device_array.
>
> I'll work through this but it's going to be tricky to tell if we get right.
> Step 1 will be closing the existing leaks and then we will have something
> consistent to build on.

Sounds good to me.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* [PATCH] clk: samsung: gs101: mark some apm UASC and XIU clocks critical
From: André Draszik @ 2024-04-30  9:54 UTC (permalink / raw)
  To: Peter Griffin, Krzysztof Kozlowski, Sylwester Nawrocki,
	Chanwoo Choi, Alim Akhtar, Michael Turquette, Stephen Boyd
  Cc: linux-arm-kernel, linux-samsung-soc, linux-clk, linux-kernel,
	Tudor Ambarus, Will McVicker, kernel-team, André Draszik

The system hangs when any of these clocks are turned off.

With the introduction of pinctrl clock support [1], the approach taken
in this clock driver for the APM clocks to rely solely on the
clk_ignore_unused kernel command line option does not work anymore and
the system hangs during boot.

gout_apm_func is a parent clock to the clocks that are going to be
handled by the pinctrl driver [2], namely
gout_apm_apbif_gpio_alive_pclk and gout_apm_apbif_gpio_far_alive_pclk.
It also is the parent to the clocks marked as critical in this commit
here (and some others that aren't relevant for this commit)). This
means that once the pinctrl driver decides to turn off clocks, the
clock framework will subsequently turn off parent clocks of those
pinctrl clocks if they have no (apparent) user. Since gout_apm_func is
the parent, and since no drivers are hooked up to it or any of its
other children, gout_apm_func will be turned off. This will cause the
system to hang, as the clocks marked as critical in this commit stop
having an input.

We might have to add a driver for these clocks, but in the meantime
let's just ensure they stay on even if siblings are turned off.

For the avoidance of doubt: This commit doesn't mean that we can boot
with clk_ignore_unused.

Link: https://lore.kernel.org/r/20240426-samsung-pinctrl-busclock-v3-0-adb8664b8a7e@linaro.org [1]
Link: https://lore.kernel.org/r/20240429-samsung-pinctrl-busclock-dts-v1-0-5e935179f3ca@linaro.org [2]
Signed-off-by: André Draszik <andre.draszik@linaro.org>
---
 drivers/clk/samsung/clk-gs101.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/clk/samsung/clk-gs101.c b/drivers/clk/samsung/clk-gs101.c
index 05129c3b2f68..e2a6a1992505 100644
--- a/drivers/clk/samsung/clk-gs101.c
+++ b/drivers/clk/samsung/clk-gs101.c
@@ -1896,16 +1896,16 @@ static const struct samsung_gate_clock apm_gate_clks[] __initconst = {
 	     CLK_CON_GAT_GOUT_BLK_APM_UID_UASC_G_SWD_IPCLKPORT_PCLK, 21, 0, 0),
 	GATE(CLK_GOUT_APM_UASC_P_APM_ACLK,
 	     "gout_apm_uasc_p_apm_aclk", "gout_apm_func",
-	     CLK_CON_GAT_GOUT_BLK_APM_UID_UASC_P_APM_IPCLKPORT_ACLK, 21, 0, 0),
+	     CLK_CON_GAT_GOUT_BLK_APM_UID_UASC_P_APM_IPCLKPORT_ACLK, 21, CLK_IS_CRITICAL, 0),
 	GATE(CLK_GOUT_APM_UASC_P_APM_PCLK,
 	     "gout_apm_uasc_p_apm_pclk", "gout_apm_func",
-	     CLK_CON_GAT_GOUT_BLK_APM_UID_UASC_P_APM_IPCLKPORT_PCLK, 21, 0, 0),
+	     CLK_CON_GAT_GOUT_BLK_APM_UID_UASC_P_APM_IPCLKPORT_PCLK, 21, CLK_IS_CRITICAL, 0),
 	GATE(CLK_GOUT_APM_WDT_APM_PCLK,
 	     "gout_apm_wdt_apm_pclk", "gout_apm_func",
 	     CLK_CON_GAT_GOUT_BLK_APM_UID_WDT_APM_IPCLKPORT_PCLK, 21, 0, 0),
 	GATE(CLK_GOUT_APM_XIU_DP_APM_ACLK,
 	     "gout_apm_xiu_dp_apm_aclk", "gout_apm_func",
-	     CLK_CON_GAT_GOUT_BLK_APM_UID_XIU_DP_APM_IPCLKPORT_ACLK, 21, 0, 0),
+	     CLK_CON_GAT_GOUT_BLK_APM_UID_XIU_DP_APM_IPCLKPORT_ACLK, 21, CLK_IS_CRITICAL, 0),
 };
 
 static const struct samsung_cmu_info apm_cmu_info __initconst = {

---
base-commit: d04466706db5e241ee026f17b5f920e50dee26b5
change-id: 20240430-gs101-apm-clocks-fb0918ceaecb

Best regards,
-- 
André Draszik <andre.draszik@linaro.org>


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related

* Re: [PATCH] arm64: dts: mediatek: mt8192-asurada: Add off-on-delay-us for pp3300_mipibrdg
From: AngeloGioacchino Del Regno @ 2024-04-30  9:52 UTC (permalink / raw)
  To: Pin-yen Lin
  Cc: Matthias Brugger,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	moderated list:ARM/Mediatek SoC support,
	moderated list:ARM/Mediatek SoC support,
	Nícolas F . R . A . Prado,
	open list:ARM/Mediatek SoC support, Hsin-Te Yuan
In-Reply-To: <CAEXTbpf2HOQj_AxHGbsgOXVF_HyKttL=z7Mi8QStcmuOS+yN7g@mail.gmail.com>

Il 30/04/24 11:32, Pin-yen Lin ha scritto:
> Hi Angelo,
> 
> On Tue, Apr 30, 2024 at 4:17 PM AngeloGioacchino Del Regno
> <angelogioacchino.delregno@collabora.com> wrote:
>>
>> Il 29/04/24 11:53, Pin-yen Lin ha scritto:
>>> Set off-on-delay-us to 500000 us for pp3300_mipibrdg to make sure it
>>> complies with the panel sequence. Explicit configuration on the
>>> regulator node is required because mt8192-asurada uses the same power
>>> supply for the panel and the anx7625 DP bridge. So powering on/off the
>>> DP bridge could break the power sequence requirement for the panel.
>>>
>>> Fixes: f9f00b1f6b9b ("arm64: dts: mediatek: asurada: Add display regulators")
>>> Signed-off-by: Pin-yen Lin <treapking@chromium.org>
>>>
>>
>> Uhm, there might be more to it - I don't think that this should ever happen.
>>
>> The regulator is refcounted, so...
>>    * Bridge on: panel goes off, but regulator doesn't turn off (refcount=1)
>>      * Panel resume -> sequence respected (refcount=2 -> wait -> more vregs, etc)
>>    * Bridge off: panel is already off (refcount=0)
>>      * Bridge resume -> refcount=1, no panel commands yet
> 
> The off-on-delay could be violated because the bridge driver does not
> check the delay.
> 
>>      * Panel resume -> refcount=2, wait -> more vregs, etc
>>
>> Can you please describe the issue that you're getting?
> 
> The symptom we observed is that the device has a small chance to
> reboot to a black panel, and we think the panel's unprepare delay (the
> time to power down completely) might not be satisfied because the
> bridge doesn't check that when it enables the regulator. Even if the
> regulator is enabled by the panel driver, the delay can also be
> violated in the following sequence:
> 
> * t=0ms, bridge on: panel goes off, but regulator doesn't turn off
> (refcount=1). The .unprepared_time in panel_edp is updated
> * t=300ms, bridge off, regulator goes off (refcount=0)
> * t=600ms, panel on, the panel driver thinks the unprepare delay
> (500ms) is satisfied, but the regulator was disabled 300ms ago.
> 
> Did I miss anything here? Or should I add more detail to the commit message?
> 

Heh, no you didn't miss anything, this time it's just me :-)

If you can please add that description to the commit message for a v2 that'd be
appreciated on my side.

In any case

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

>>
>> Cheers,
>> Angelo
>>
> Regards,
> Pin-yen
> 
>>> ---
>>>
>>>    arch/arm64/boot/dts/mediatek/mt8192-asurada.dtsi | 1 +
>>>    1 file changed, 1 insertion(+)
>>>
>>> diff --git a/arch/arm64/boot/dts/mediatek/mt8192-asurada.dtsi b/arch/arm64/boot/dts/mediatek/mt8192-asurada.dtsi
>>> index 7a704246678f..08d71ddf3668 100644
>>> --- a/arch/arm64/boot/dts/mediatek/mt8192-asurada.dtsi
>>> +++ b/arch/arm64/boot/dts/mediatek/mt8192-asurada.dtsi
>>> @@ -147,6 +147,7 @@ pp3300_mipibrdg: regulator-3v3-mipibrdg {
>>>                regulator-boot-on;
>>>                gpio = <&pio 127 GPIO_ACTIVE_HIGH>;
>>>                vin-supply = <&pp3300_g>;
>>> +             off-on-delay-us = <500000>;
>>>        };
>>>
>>>        /* separately switched 3.3V power rail */
>>


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* [PATCH v2 1/4] arm64: dts: exynos: gs101: specify bus clock for pinctrl (far) alive
From: André Draszik @ 2024-04-30  9:49 UTC (permalink / raw)
  To: Peter Griffin, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
	Alim Akhtar
  Cc: Tudor Ambarus, Will McVicker, Sam Protsenko, kernel-team,
	linux-arm-kernel, linux-samsung-soc, devicetree, linux-kernel,
	André Draszik
In-Reply-To: <20240430-samsung-pinctrl-busclock-dts-v2-0-14fc988139dd@linaro.org>

This bus clock is needed for pinctrl register access to work. Add it.

Signed-off-by: André Draszik <andre.draszik@linaro.org>
---
 arch/arm64/boot/dts/exynos/google/gs101.dtsi | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm64/boot/dts/exynos/google/gs101.dtsi b/arch/arm64/boot/dts/exynos/google/gs101.dtsi
index e3b068c1a2c1..f2c7c2a4ce1c 100644
--- a/arch/arm64/boot/dts/exynos/google/gs101.dtsi
+++ b/arch/arm64/boot/dts/exynos/google/gs101.dtsi
@@ -1348,6 +1348,8 @@ pmu_system_controller: system-controller@17460000 {
 		pinctrl_gpio_alive: pinctrl@174d0000 {
 			compatible = "google,gs101-pinctrl";
 			reg = <0x174d0000 0x00001000>;
+			clocks = <&cmu_apm CLK_GOUT_APM_APBIF_GPIO_ALIVE_PCLK>;
+			clock-names = "pclk";
 
 			wakeup-interrupt-controller {
 				compatible = "google,gs101-wakeup-eint",
@@ -1359,6 +1361,8 @@ wakeup-interrupt-controller {
 		pinctrl_far_alive: pinctrl@174e0000 {
 			compatible = "google,gs101-pinctrl";
 			reg = <0x174e0000 0x00001000>;
+			clocks = <&cmu_apm CLK_GOUT_APM_APBIF_GPIO_FAR_ALIVE_PCLK>;
+			clock-names = "pclk";
 
 			wakeup-interrupt-controller {
 				compatible = "google,gs101-wakeup-eint",

-- 
2.44.0.769.g3c40516874-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related

* [PATCH v2 3/4] arm64: dts: exynos: gs101: specify bus clock for pinctrl_hsi2
From: André Draszik @ 2024-04-30  9:49 UTC (permalink / raw)
  To: Peter Griffin, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
	Alim Akhtar
  Cc: Tudor Ambarus, Will McVicker, Sam Protsenko, kernel-team,
	linux-arm-kernel, linux-samsung-soc, devicetree, linux-kernel,
	André Draszik
In-Reply-To: <20240430-samsung-pinctrl-busclock-dts-v2-0-14fc988139dd@linaro.org>

This bus clock is needed for pinctrl register access to work. Add it.

Signed-off-by: André Draszik <andre.draszik@linaro.org>
---
 arch/arm64/boot/dts/exynos/google/gs101.dtsi | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm64/boot/dts/exynos/google/gs101.dtsi b/arch/arm64/boot/dts/exynos/google/gs101.dtsi
index 8d4216cbab2e..f8fcbbb06e7b 100644
--- a/arch/arm64/boot/dts/exynos/google/gs101.dtsi
+++ b/arch/arm64/boot/dts/exynos/google/gs101.dtsi
@@ -1327,6 +1327,8 @@ cmu_hsi2: clock-controller@14400000 {
 		pinctrl_hsi2: pinctrl@14440000 {
 			compatible = "google,gs101-pinctrl";
 			reg = <0x14440000 0x00001000>;
+			clocks = <&cmu_hsi2 CLK_GOUT_HSI2_GPIO_HSI2_PCLK>;
+			clock-names = "pclk";
 			interrupts = <GIC_SPI 503 IRQ_TYPE_LEVEL_HIGH 0>;
 		};
 

-- 
2.44.0.769.g3c40516874-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related

* [PATCH v2 0/4] hook up pin controller clocks on Google Tensor gs101
From: André Draszik @ 2024-04-30  9:49 UTC (permalink / raw)
  To: Peter Griffin, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
	Alim Akhtar
  Cc: Tudor Ambarus, Will McVicker, Sam Protsenko, kernel-team,
	linux-arm-kernel, linux-samsung-soc, devicetree, linux-kernel,
	André Draszik

This series hooks up the individual clocks for each pin controller in the
gs101 DTS.

On Google Tensor gs101 there are separate bus clocks / gates each for each
pinctrl instance. To be able to access each pinctrl instance's registers,
this bus clock needs to be running, otherwise register access will hang.

The driver update to support this extra clock has been proposed in
https://lore.kernel.org/r/20240426-samsung-pinctrl-busclock-v3-0-adb8664b8a7e@linaro.org

This series depends on:
* hsi2 series:
  https://lore.kernel.org/r/20240429-hsi0-gs101-v3-0-f233be0a2455@linaro.org
* pin controller clock support:
  https://lore.kernel.org/r/20240426-samsung-pinctrl-busclock-v3-0-adb8664b8a7e@linaro.org

Signed-off-by: André Draszik <andre.draszik@linaro.org>
---
Changes in v2:
- use <0> instead of a placeholder clock (Krzysztof)
- Link to v1: https://lore.kernel.org/r/20240429-samsung-pinctrl-busclock-dts-v1-0-5e935179f3ca@linaro.org

---
André Draszik (4):
      arm64: dts: exynos: gs101: specify bus clock for pinctrl (far) alive
      arm64: dts: exynos: gs101: specify bus clock for pinctrl_peric[01]
      arm64: dts: exynos: gs101: specify bus clock for pinctrl_hsi2
      arm64: dts: exynos: gs101: specify empty clocks for remaining pinctrl

 arch/arm64/boot/dts/exynos/google/gs101.dtsi | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)
---
base-commit: d04466706db5e241ee026f17b5f920e50dee26b5
change-id: 20240429-samsung-pinctrl-busclock-dts-46b223471541

Best regards,
-- 
André Draszik <andre.draszik@linaro.org>


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* [PATCH v2 2/4] arm64: dts: exynos: gs101: specify bus clock for pinctrl_peric[01]
From: André Draszik @ 2024-04-30  9:49 UTC (permalink / raw)
  To: Peter Griffin, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
	Alim Akhtar
  Cc: Tudor Ambarus, Will McVicker, Sam Protsenko, kernel-team,
	linux-arm-kernel, linux-samsung-soc, devicetree, linux-kernel,
	André Draszik
In-Reply-To: <20240430-samsung-pinctrl-busclock-dts-v2-0-14fc988139dd@linaro.org>

This bus clock is needed for pinctrl register access to work. Add it.

Signed-off-by: André Draszik <andre.draszik@linaro.org>
---
 arch/arm64/boot/dts/exynos/google/gs101.dtsi | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm64/boot/dts/exynos/google/gs101.dtsi b/arch/arm64/boot/dts/exynos/google/gs101.dtsi
index f2c7c2a4ce1c..8d4216cbab2e 100644
--- a/arch/arm64/boot/dts/exynos/google/gs101.dtsi
+++ b/arch/arm64/boot/dts/exynos/google/gs101.dtsi
@@ -370,6 +370,8 @@ sysreg_peric0: syscon@10820000 {
 		pinctrl_peric0: pinctrl@10840000 {
 			compatible = "google,gs101-pinctrl";
 			reg = <0x10840000 0x00001000>;
+			clocks = <&cmu_peric0 CLK_GOUT_PERIC0_GPIO_PERIC0_PCLK>;
+			clock-names = "pclk";
 			interrupts = <GIC_SPI 625 IRQ_TYPE_LEVEL_HIGH 0>;
 		};
 
@@ -914,6 +916,8 @@ sysreg_peric1: syscon@10c20000 {
 		pinctrl_peric1: pinctrl@10c40000 {
 			compatible = "google,gs101-pinctrl";
 			reg = <0x10c40000 0x00001000>;
+			clocks = <&cmu_peric1 CLK_GOUT_PERIC1_GPIO_PERIC1_PCLK>;
+			clock-names = "pclk";
 			interrupts = <GIC_SPI 644 IRQ_TYPE_LEVEL_HIGH 0>;
 		};
 

-- 
2.44.0.769.g3c40516874-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related

* [PATCH v2 4/4] arm64: dts: exynos: gs101: specify empty clocks for remaining pinctrl
From: André Draszik @ 2024-04-30  9:49 UTC (permalink / raw)
  To: Peter Griffin, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
	Alim Akhtar
  Cc: Tudor Ambarus, Will McVicker, Sam Protsenko, kernel-team,
	linux-arm-kernel, linux-samsung-soc, devicetree, linux-kernel,
	André Draszik
In-Reply-To: <20240430-samsung-pinctrl-busclock-dts-v2-0-14fc988139dd@linaro.org>

The pinctrl instances hsi1, gsactrl, and gsacore need a clock for
register access to work.

Since we haven't implemented the relevant CMUs for the clocks required
by these instances just add empty clocks for now so as to make the DT
pass the validation checks.
Once the clocks are implmented in the gs101 clock driver, these should
be updated then.

Signed-off-by: André Draszik <andre.draszik@linaro.org>

---
v2: use empty clock instead of placeholder
---
 arch/arm64/boot/dts/exynos/google/gs101.dtsi | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/arch/arm64/boot/dts/exynos/google/gs101.dtsi b/arch/arm64/boot/dts/exynos/google/gs101.dtsi
index f8fcbbb06e7b..c8a5eb8c7d45 100644
--- a/arch/arm64/boot/dts/exynos/google/gs101.dtsi
+++ b/arch/arm64/boot/dts/exynos/google/gs101.dtsi
@@ -1309,6 +1309,9 @@ usbdrd31_dwc3: usb@0 {
 		pinctrl_hsi1: pinctrl@11840000 {
 			compatible = "google,gs101-pinctrl";
 			reg = <0x11840000 0x00001000>;
+			/* TODO: update once support for this CMU exists */
+			clocks = <0>;
+			clock-names = "pclk";
 			interrupts = <GIC_SPI 471 IRQ_TYPE_LEVEL_HIGH 0>;
 		};
 
@@ -1380,11 +1383,17 @@ wakeup-interrupt-controller {
 		pinctrl_gsactrl: pinctrl@17940000 {
 			compatible = "google,gs101-pinctrl";
 			reg = <0x17940000 0x00001000>;
+			/* TODO: update once support for this CMU exists */
+			clocks = <0>;
+			clock-names = "pclk";
 		};
 
 		pinctrl_gsacore: pinctrl@17a80000 {
 			compatible = "google,gs101-pinctrl";
 			reg = <0x17a80000 0x00001000>;
+			/* TODO: update once support for this CMU exists */
+			clocks = <0>;
+			clock-names = "pclk";
 		};
 
 		cmu_top: clock-controller@1e080000 {

-- 
2.44.0.769.g3c40516874-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related

* [PATCH 14/17] of: dynamic: Introduce of_changeset_add_prop_bool()
From: Herve Codina @ 2024-04-30  8:37 UTC (permalink / raw)
  To: Herve Codina, Thomas Gleixner, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Lee Jones, Arnd Bergmann, Horatiu Vultur,
	UNGLinuxDriver, Andrew Lunn, Heiner Kallweit, Russell King,
	Saravana Kannan, Bjorn Helgaas, Philipp Zabel, Lars Povlsen,
	Steen Hegelund, Daniel Machon, Alexandre Belloni
  Cc: linux-kernel, devicetree, netdev, linux-pci, linux-arm-kernel,
	Allan Nielsen, Steen Hegelund, Luca Ceresoli, Thomas Petazzoni
In-Reply-To: <20240430083730.134918-1-herve.codina@bootlin.com>

APIs to add some properties in a changeset exist but nothing to add a DT
boolean property (i.e. a property without any values).

Fill this lack with of_changeset_add_prop_bool().

Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
 drivers/of/dynamic.c | 25 +++++++++++++++++++++++++
 include/linux/of.h   |  3 +++
 2 files changed, 28 insertions(+)

diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
index 4d57a4e34105..37d3b0a272dc 100644
--- a/drivers/of/dynamic.c
+++ b/drivers/of/dynamic.c
@@ -1052,3 +1052,28 @@ int of_changeset_add_prop_u32_array(struct of_changeset *ocs,
 	return ret;
 }
 EXPORT_SYMBOL_GPL(of_changeset_add_prop_u32_array);
+
+/**
+ * of_changeset_add_prop_bool - Add a boolean property (i.e. a property without
+ * any values) to a changeset.
+ *
+ * @ocs:	changeset pointer
+ * @np:		device node pointer
+ * @prop_name:	name of the property to be added
+ *
+ * Create a boolean property and add it to a changeset.
+ *
+ * Return: 0 on success, a negative error value in case of an error.
+ */
+int of_changeset_add_prop_bool(struct of_changeset *ocs, struct device_node *np,
+			       const char *prop_name)
+{
+	struct property prop;
+
+	prop.name = (char *)prop_name;
+	prop.length = 0;
+	prop.value = NULL;
+
+	return of_changeset_add_prop_helper(ocs, np, &prop);
+}
+EXPORT_SYMBOL_GPL(of_changeset_add_prop_bool);
diff --git a/include/linux/of.h b/include/linux/of.h
index a0bedd038a05..9633199a954a 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -1652,6 +1652,9 @@ static inline int of_changeset_add_prop_u32(struct of_changeset *ocs,
 	return of_changeset_add_prop_u32_array(ocs, np, prop_name, &val, 1);
 }
 
+int of_changeset_add_prop_bool(struct of_changeset *ocs, struct device_node *np,
+			       const char *prop_name);
+
 #else /* CONFIG_OF_DYNAMIC */
 static inline int of_reconfig_notifier_register(struct notifier_block *nb)
 {
-- 
2.44.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related

* [PATCH 13/17] MAINTAINERS: Add the Microchip LAN966x OIC driver entry
From: Herve Codina @ 2024-04-30  8:37 UTC (permalink / raw)
  To: Herve Codina, Thomas Gleixner, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Lee Jones, Arnd Bergmann, Horatiu Vultur,
	UNGLinuxDriver, Andrew Lunn, Heiner Kallweit, Russell King,
	Saravana Kannan, Bjorn Helgaas, Philipp Zabel, Lars Povlsen,
	Steen Hegelund, Daniel Machon, Alexandre Belloni
  Cc: linux-kernel, devicetree, netdev, linux-pci, linux-arm-kernel,
	Allan Nielsen, Steen Hegelund, Luca Ceresoli, Thomas Petazzoni
In-Reply-To: <20240430083730.134918-1-herve.codina@bootlin.com>

After contributing the driver, add myself as the maintainer for the
Microchip LAN966x OIC driver.

Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
 MAINTAINERS | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index ebf03f5f0619..a15f19008d6e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -14465,6 +14465,12 @@ L:	netdev@vger.kernel.org
 S:	Maintained
 F:	drivers/net/ethernet/microchip/lan966x/*
 
+MICROCHIP LAN966X OIC DRIVER
+M:	Herve Codina <herve.codina@bootlin.com>
+S:	Maintained
+F:	Documentation/devicetree/bindings/interrupt-controller/microchip,lan966x-oic.yaml
+F:	drivers/irqchip/irq-lan966x-oic.c
+
 MICROCHIP LCDFB DRIVER
 M:	Nicolas Ferre <nicolas.ferre@microchip.com>
 L:	linux-fbdev@vger.kernel.org
-- 
2.44.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related

* [PATCH 12/17] irqchip: Add support for LAN966x OIC
From: Herve Codina @ 2024-04-30  8:37 UTC (permalink / raw)
  To: Herve Codina, Thomas Gleixner, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Lee Jones, Arnd Bergmann, Horatiu Vultur,
	UNGLinuxDriver, Andrew Lunn, Heiner Kallweit, Russell King,
	Saravana Kannan, Bjorn Helgaas, Philipp Zabel, Lars Povlsen,
	Steen Hegelund, Daniel Machon, Alexandre Belloni
  Cc: linux-kernel, devicetree, netdev, linux-pci, linux-arm-kernel,
	Allan Nielsen, Steen Hegelund, Luca Ceresoli, Thomas Petazzoni
In-Reply-To: <20240430083730.134918-1-herve.codina@bootlin.com>

The Microchip LAN966x outband interrupt controller (OIC) maps the
internal interrupt sources of the LAN966x device to an external
interrupt.
When the LAN966x device is used as a PCI device, the external interrupt
is routed to the PCI interrupt.

Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
 drivers/irqchip/Kconfig           |  12 ++
 drivers/irqchip/Makefile          |   1 +
 drivers/irqchip/irq-lan966x-oic.c | 301 ++++++++++++++++++++++++++++++
 3 files changed, 314 insertions(+)
 create mode 100644 drivers/irqchip/irq-lan966x-oic.c

diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index 72c07a12f5e1..973eebc8d1d1 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -169,6 +169,18 @@ config IXP4XX_IRQ
 	select IRQ_DOMAIN
 	select SPARSE_IRQ
 
+config LAN966X_OIC
+	tristate "Microchip LAN966x OIC Support"
+	select GENERIC_IRQ_CHIP
+	select IRQ_DOMAIN
+	help
+	  Enable support for the LAN966x Outbound Interrupt Controller.
+	  This controller is present on the Microchip LAN966x PCI device and
+	  maps the internal interrupts sources to PCIe interrupt.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called irq-lan966x-oic.
+
 config MADERA_IRQ
 	tristate
 
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index ec4a18380998..762d3078aa3b 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -101,6 +101,7 @@ obj-$(CONFIG_IMX_IRQSTEER)		+= irq-imx-irqsteer.o
 obj-$(CONFIG_IMX_INTMUX)		+= irq-imx-intmux.o
 obj-$(CONFIG_IMX_MU_MSI)		+= irq-imx-mu-msi.o
 obj-$(CONFIG_MADERA_IRQ)		+= irq-madera.o
+obj-$(CONFIG_LAN966X_OIC)		+= irq-lan966x-oic.o
 obj-$(CONFIG_LS1X_IRQ)			+= irq-ls1x.o
 obj-$(CONFIG_TI_SCI_INTR_IRQCHIP)	+= irq-ti-sci-intr.o
 obj-$(CONFIG_TI_SCI_INTA_IRQCHIP)	+= irq-ti-sci-inta.o
diff --git a/drivers/irqchip/irq-lan966x-oic.c b/drivers/irqchip/irq-lan966x-oic.c
new file mode 100644
index 000000000000..342f0dbf16e3
--- /dev/null
+++ b/drivers/irqchip/irq-lan966x-oic.c
@@ -0,0 +1,301 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Driver for the Microchip LAN966x outbound interrupt controller
+ *
+ * Copyright (c) 2024 Technology Inc. and its subsidiaries.
+ *
+ * Authors:
+ *	Horatiu Vultur <horatiu.vultur@microchip.com>
+ *	Clément Léger <clement.leger@bootlin.com>
+ *	Herve Codina <herve.codina@bootlin.com>
+ */
+
+#include <linux/bitops.h>
+#include <linux/build_bug.h>
+#include <linux/interrupt.h>
+#include <linux/irqchip/chained_irq.h>
+#include <linux/irqchip.h>
+#include <linux/irq.h>
+#include <linux/iopoll.h>
+#include <linux/mfd/core.h>
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/delay.h>
+
+struct lan966x_oic_chip_regs {
+	int reg_off_ena_set;
+	int reg_off_ena_clr;
+	int reg_off_sticky;
+	int reg_off_ident;
+	int reg_off_map;
+};
+
+struct lan966x_oic_data {
+	struct irq_domain *domain;
+	void __iomem *regs;
+	int irq;
+};
+
+#define LAN966X_OIC_NR_IRQ 86
+
+/* Interrupt sticky status */
+#define LAN966X_OIC_INTR_STICKY		0x30
+#define LAN966X_OIC_INTR_STICKY1	0x34
+#define LAN966X_OIC_INTR_STICKY2	0x38
+
+/* Interrupt enable */
+#define LAN966X_OIC_INTR_ENA		0x48
+#define LAN966X_OIC_INTR_ENA1		0x4c
+#define LAN966X_OIC_INTR_ENA2		0x50
+
+/* Atomic clear of interrupt enable */
+#define LAN966X_OIC_INTR_ENA_CLR	0x54
+#define LAN966X_OIC_INTR_ENA_CLR1	0x58
+#define LAN966X_OIC_INTR_ENA_CLR2	0x5c
+
+/* Atomic set of interrupt */
+#define LAN966X_OIC_INTR_ENA_SET	0x60
+#define LAN966X_OIC_INTR_ENA_SET1	0x64
+#define LAN966X_OIC_INTR_ENA_SET2	0x68
+
+/* Mapping of source to destination interrupts (_n = 0..8) */
+#define LAN966X_OIC_DST_INTR_MAP(_n)	0x78
+#define LAN966X_OIC_DST_INTR_MAP1(_n)	0x9c
+#define LAN966X_OIC_DST_INTR_MAP2(_n)	0xc0
+
+/* Currently active interrupt sources per destination (_n = 0..8) */
+#define LAN966X_OIC_DST_INTR_IDENT(_n)	0xe4
+#define LAN966X_OIC_DST_INTR_IDENT1(_n)	0x108
+#define LAN966X_OIC_DST_INTR_IDENT2(_n)	0x12c
+
+static unsigned int lan966x_oic_irq_startup(struct irq_data *data)
+{
+	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(data);
+	struct irq_chip_type *ct = irq_data_get_chip_type(data);
+	struct lan966x_oic_chip_regs *chip_regs = gc->private;
+	u32 map;
+
+	irq_gc_lock(gc);
+
+	/* Map the source interrupt to the destination */
+	map = irq_reg_readl(gc, chip_regs->reg_off_map);
+	map |= data->mask;
+	irq_reg_writel(gc, map, chip_regs->reg_off_map);
+
+	irq_gc_unlock(gc);
+
+	ct->chip.irq_ack(data);
+	ct->chip.irq_unmask(data);
+
+	return 0;
+}
+
+static void lan966x_oic_irq_shutdown(struct irq_data *data)
+{
+	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(data);
+	struct irq_chip_type *ct = irq_data_get_chip_type(data);
+	struct lan966x_oic_chip_regs *chip_regs = gc->private;
+	u32 map;
+
+	ct->chip.irq_mask(data);
+
+	irq_gc_lock(gc);
+
+	/* Unmap the interrupt */
+	map = irq_reg_readl(gc, chip_regs->reg_off_map);
+	map &= ~data->mask;
+	irq_reg_writel(gc, map, chip_regs->reg_off_map);
+
+	irq_gc_unlock(gc);
+}
+
+static int lan966x_oic_irq_set_type(struct irq_data *data, unsigned int flow_type)
+{
+	if (flow_type != IRQ_TYPE_LEVEL_HIGH) {
+		pr_err("lan966x oic doesn't support flow type %d\n", flow_type);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static void lan966x_oic_irq_handler_domain(struct irq_domain *d, u32 first_irq)
+{
+	struct irq_chip_generic *gc = irq_get_domain_generic_chip(d, first_irq);
+	struct lan966x_oic_chip_regs *chip_regs = gc->private;
+	unsigned long ident;
+	unsigned int hwirq;
+
+	ident = irq_reg_readl(gc, chip_regs->reg_off_ident);
+	if (!ident)
+		return;
+
+	for_each_set_bit(hwirq, &ident, 32)
+		generic_handle_domain_irq(d, hwirq + first_irq);
+}
+
+static void lan966x_oic_irq_handler(struct irq_desc *desc)
+{
+	struct irq_domain *d = irq_desc_get_handler_data(desc);
+	struct irq_chip *chip = irq_desc_get_chip(desc);
+
+	chained_irq_enter(chip, desc);
+	lan966x_oic_irq_handler_domain(d, 0);
+	lan966x_oic_irq_handler_domain(d, 32);
+	lan966x_oic_irq_handler_domain(d, 64);
+	chained_irq_exit(chip, desc);
+}
+
+static struct lan966x_oic_chip_regs lan966x_oic_chip_regs[3] = {
+	{
+		.reg_off_ena_set = LAN966X_OIC_INTR_ENA_SET,
+		.reg_off_ena_clr = LAN966X_OIC_INTR_ENA_CLR,
+		.reg_off_sticky = LAN966X_OIC_INTR_STICKY,
+		.reg_off_ident = LAN966X_OIC_DST_INTR_IDENT(0),
+		.reg_off_map = LAN966X_OIC_DST_INTR_MAP(0),
+	}, {
+		.reg_off_ena_set = LAN966X_OIC_INTR_ENA_SET1,
+		.reg_off_ena_clr = LAN966X_OIC_INTR_ENA_CLR1,
+		.reg_off_sticky = LAN966X_OIC_INTR_STICKY1,
+		.reg_off_ident = LAN966X_OIC_DST_INTR_IDENT1(0),
+		.reg_off_map = LAN966X_OIC_DST_INTR_MAP1(0),
+	}, {
+		.reg_off_ena_set = LAN966X_OIC_INTR_ENA_SET2,
+		.reg_off_ena_clr = LAN966X_OIC_INTR_ENA_CLR2,
+		.reg_off_sticky = LAN966X_OIC_INTR_STICKY2,
+		.reg_off_ident = LAN966X_OIC_DST_INTR_IDENT2(0),
+		.reg_off_map = LAN966X_OIC_DST_INTR_MAP2(0),
+	}
+};
+
+static void lan966x_oic_chip_init(struct lan966x_oic_data *lan966x_oic,
+				  struct irq_chip_generic *gc,
+				  struct lan966x_oic_chip_regs *chip_regs)
+{
+	gc->reg_base = lan966x_oic->regs;
+	gc->chip_types[0].regs.enable = chip_regs->reg_off_ena_set;
+	gc->chip_types[0].regs.disable = chip_regs->reg_off_ena_clr;
+	gc->chip_types[0].regs.ack = chip_regs->reg_off_sticky;
+	gc->chip_types[0].chip.irq_startup = lan966x_oic_irq_startup;
+	gc->chip_types[0].chip.irq_shutdown = lan966x_oic_irq_shutdown;
+	gc->chip_types[0].chip.irq_set_type = lan966x_oic_irq_set_type;
+	gc->chip_types[0].chip.irq_mask = irq_gc_mask_disable_reg;
+	gc->chip_types[0].chip.irq_unmask = irq_gc_unmask_enable_reg;
+	gc->chip_types[0].chip.irq_ack = irq_gc_ack_set_bit;
+	gc->private = chip_regs;
+
+	/* Disable all interrupts handled by this chip */
+	irq_reg_writel(gc, ~0, chip_regs->reg_off_ena_clr);
+}
+
+static void lan966x_oic_chip_exit(struct irq_chip_generic *gc)
+{
+	/* Disable and ack all interrupts handled by this chip */
+	irq_reg_writel(gc, ~0, gc->chip_types[0].regs.disable);
+	irq_reg_writel(gc, ~0, gc->chip_types[0].regs.ack);
+}
+
+static int lan966x_oic_probe(struct platform_device *pdev)
+{
+	struct device_node *node = pdev->dev.of_node;
+	struct lan966x_oic_data *lan966x_oic;
+	struct device *dev = &pdev->dev;
+	struct irq_chip_generic *gc;
+	int ret;
+	int i;
+
+	lan966x_oic = devm_kmalloc(dev, sizeof(*lan966x_oic), GFP_KERNEL);
+	if (!lan966x_oic)
+		return -ENOMEM;
+
+	lan966x_oic->regs = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(lan966x_oic->regs))
+		return dev_err_probe(dev, PTR_ERR(lan966x_oic->regs),
+				     "failed to map resource\n");
+
+	lan966x_oic->domain = irq_domain_alloc_linear(of_node_to_fwnode(node),
+						      LAN966X_OIC_NR_IRQ,
+						      &irq_generic_chip_ops, NULL);
+	if (!lan966x_oic->domain) {
+		dev_err(dev, "failed to create an IRQ domain\n");
+		return -EINVAL;
+	}
+
+	lan966x_oic->irq = platform_get_irq(pdev, 0);
+	if (lan966x_oic->irq < 0) {
+		dev_err_probe(dev, lan966x_oic->irq, "failed to get the IRQ\n");
+		goto err_domain_free;
+	}
+
+	ret = irq_alloc_domain_generic_chips(lan966x_oic->domain, 32, 1, "lan966x-oic",
+					     handle_level_irq, 0, 0, 0);
+	if (ret) {
+		dev_err_probe(dev, ret, "failed to alloc irq domain gc\n");
+		goto err_domain_free;
+	}
+
+	/* Init chips */
+	BUILD_BUG_ON(DIV_ROUND_UP(LAN966X_OIC_NR_IRQ, 32) != ARRAY_SIZE(lan966x_oic_chip_regs));
+	for (i = 0; i < ARRAY_SIZE(lan966x_oic_chip_regs); i++) {
+		gc = irq_get_domain_generic_chip(lan966x_oic->domain, i * 32);
+		lan966x_oic_chip_init(lan966x_oic, gc, &lan966x_oic_chip_regs[i]);
+	}
+
+	irq_set_chained_handler_and_data(lan966x_oic->irq, lan966x_oic_irq_handler,
+					 lan966x_oic->domain);
+
+	irq_domain_publish(lan966x_oic->domain);
+	platform_set_drvdata(pdev, lan966x_oic);
+	return 0;
+
+err_domain_free:
+	irq_domain_free(lan966x_oic->domain);
+	return ret;
+}
+
+static void lan966x_oic_remove(struct platform_device *pdev)
+{
+	struct lan966x_oic_data *lan966x_oic = platform_get_drvdata(pdev);
+	struct irq_chip_generic *gc;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(lan966x_oic_chip_regs); i++) {
+		gc = irq_get_domain_generic_chip(lan966x_oic->domain, i * 32);
+		lan966x_oic_chip_exit(gc);
+	}
+
+	irq_set_chained_handler_and_data(lan966x_oic->irq, NULL, NULL);
+
+	for (i = 0; i < LAN966X_OIC_NR_IRQ; i++)
+		irq_dispose_mapping(irq_find_mapping(lan966x_oic->domain, i));
+
+	irq_domain_unpublish(lan966x_oic->domain);
+
+	for (i = 0; i < ARRAY_SIZE(lan966x_oic_chip_regs); i++) {
+		gc = irq_get_domain_generic_chip(lan966x_oic->domain, i * 32);
+		irq_remove_generic_chip(gc, ~0, 0, 0);
+	}
+
+	kfree(lan966x_oic->domain->gc);
+	irq_domain_free(lan966x_oic->domain);
+}
+
+static const struct of_device_id lan966x_oic_of_match[] = {
+	{ .compatible = "microchip,lan966x-oic" },
+	{} /* sentinel */
+};
+MODULE_DEVICE_TABLE(of, lan966x_oic_of_match);
+
+static struct platform_driver lan966x_oic_driver = {
+	.probe = lan966x_oic_probe,
+	.remove_new = lan966x_oic_remove,
+	.driver = {
+		.name = "lan966x-oic",
+		.of_match_table = lan966x_oic_of_match,
+	},
+};
+module_platform_driver(lan966x_oic_driver);
+
+MODULE_AUTHOR("Herve Codina <herve.codina@bootlin.com>");
+MODULE_DESCRIPTION("Microchip LAN966x OIC driver");
+MODULE_LICENSE("GPL");
-- 
2.44.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related

* [PATCH 11/17] irqdomain: Introduce irq_domain_alloc() and irq_domain_publish()
From: Herve Codina @ 2024-04-30  8:37 UTC (permalink / raw)
  To: Herve Codina, Thomas Gleixner, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Lee Jones, Arnd Bergmann, Horatiu Vultur,
	UNGLinuxDriver, Andrew Lunn, Heiner Kallweit, Russell King,
	Saravana Kannan, Bjorn Helgaas, Philipp Zabel, Lars Povlsen,
	Steen Hegelund, Daniel Machon, Alexandre Belloni
  Cc: linux-kernel, devicetree, netdev, linux-pci, linux-arm-kernel,
	Allan Nielsen, Steen Hegelund, Luca Ceresoli, Thomas Petazzoni
In-Reply-To: <20240430083730.134918-1-herve.codina@bootlin.com>

The irq_domain_add_*() family functions create an irq_domain and also
publish this newly created to domain. Once an irq_domain is published,
consumers can request IRQ in order to use them.

Some interrupt controller drivers have to perform some more operations
with the created irq_domain in order to have it ready to be used.
For instance:
  - Allocate generic irq chips with irq_alloc_domain_generic_chips()
  - Retrieve the generic irq chips with irq_get_domain_generic_chip()
  - Initialize retrieved chips: set register base address and offsets,
    set several hooks such as irq_mask, irq_unmask, ...

To avoid a window where the domain is published but not yet ready to be
used, introduce irq_domain_alloc_*() family functions to create the
irq_domain and irq_domain_publish() to publish the irq_domain.
With this new functions, any additional initialisation can then be done
between the call creating the irq_domain and the call publishing it.

Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
 include/linux/irqdomain.h | 16 +++++++
 kernel/irq/irqdomain.c    | 91 ++++++++++++++++++++++++++++-----------
 2 files changed, 82 insertions(+), 25 deletions(-)

diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h
index 21ecf582a0fe..86203e7e6659 100644
--- a/include/linux/irqdomain.h
+++ b/include/linux/irqdomain.h
@@ -257,6 +257,22 @@ static inline struct fwnode_handle *irq_domain_alloc_fwnode(phys_addr_t *pa)
 }
 
 void irq_domain_free_fwnode(struct fwnode_handle *fwnode);
+struct irq_domain *irq_domain_alloc(struct fwnode_handle *fwnode, unsigned int size,
+				    irq_hw_number_t hwirq_max, int direct_max,
+				    const struct irq_domain_ops *ops,
+				    void *host_data);
+
+static inline struct irq_domain *irq_domain_alloc_linear(struct fwnode_handle *fwnode,
+							 unsigned int size,
+							 const struct irq_domain_ops *ops,
+							 void *host_data)
+{
+	return irq_domain_alloc(fwnode, size, size, 0, ops, host_data);
+}
+
+void irq_domain_free(struct irq_domain *domain);
+void irq_domain_publish(struct irq_domain *domain);
+void irq_domain_unpublish(struct irq_domain *domain);
 struct irq_domain *__irq_domain_add(struct fwnode_handle *fwnode, unsigned int size,
 				    irq_hw_number_t hwirq_max, int direct_max,
 				    const struct irq_domain_ops *ops,
diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index 1ed8c4d3cf2e..ed353789fb27 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -231,7 +231,38 @@ static struct irq_domain *__irq_domain_create(struct fwnode_handle *fwnode,
 	return domain;
 }
 
-static void __irq_domain_publish(struct irq_domain *domain)
+struct irq_domain *irq_domain_alloc(struct fwnode_handle *fwnode, unsigned int size,
+				    irq_hw_number_t hwirq_max, int direct_max,
+				    const struct irq_domain_ops *ops,
+				    void *host_data)
+{
+	return __irq_domain_create(fwnode, size, hwirq_max, direct_max, ops,
+				   host_data);
+}
+EXPORT_SYMBOL_GPL(irq_domain_alloc);
+
+/**
+ * irq_domain_free() - Free an irq domain.
+ * @domain: domain to free
+ *
+ * This routine is used to free an irq domain. The caller must ensure
+ * that the domain is not published.
+ */
+void irq_domain_free(struct irq_domain *domain)
+{
+	fwnode_dev_initialized(domain->fwnode, false);
+	fwnode_handle_put(domain->fwnode);
+	if (domain->flags & IRQ_DOMAIN_NAME_ALLOCATED)
+		kfree(domain->name);
+	kfree(domain);
+}
+EXPORT_SYMBOL_GPL(irq_domain_free);
+
+/**
+ * irq_domain_publish() - Publish an irq domain.
+ * @domain: domain to publish
+ */
+void irq_domain_publish(struct irq_domain *domain)
 {
 	mutex_lock(&irq_domain_mutex);
 	debugfs_add_domain_dir(domain);
@@ -240,6 +271,36 @@ static void __irq_domain_publish(struct irq_domain *domain)
 
 	pr_debug("Added domain %s\n", domain->name);
 }
+EXPORT_SYMBOL_GPL(irq_domain_publish);
+
+/**
+ * irq_domain_unpublish() - Unpublish an irq domain.
+ * @domain: domain to unpublish
+ *
+ * This routine is used to unpublish an irq domain. The caller must ensure
+ * that all mappings within the domain have been disposed of prior to
+ * use, depending on the revmap type.
+ */
+void irq_domain_unpublish(struct irq_domain *domain)
+{
+	mutex_lock(&irq_domain_mutex);
+	debugfs_remove_domain_dir(domain);
+
+	WARN_ON(!radix_tree_empty(&domain->revmap_tree));
+
+	list_del(&domain->link);
+
+	/*
+	 * If the going away domain is the default one, reset it.
+	 */
+	if (unlikely(irq_default_domain == domain))
+		irq_set_default_host(NULL);
+
+	mutex_unlock(&irq_domain_mutex);
+
+	pr_debug("Removed domain %s\n", domain->name);
+}
+EXPORT_SYMBOL_GPL(irq_domain_unpublish);
 
 /**
  * __irq_domain_add() - Allocate a new irq_domain data structure
@@ -264,7 +325,7 @@ struct irq_domain *__irq_domain_add(struct fwnode_handle *fwnode, unsigned int s
 	domain = __irq_domain_create(fwnode, size, hwirq_max, direct_max,
 				     ops, host_data);
 	if (domain)
-		__irq_domain_publish(domain);
+		irq_domain_publish(domain);
 
 	return domain;
 }
@@ -280,28 +341,8 @@ EXPORT_SYMBOL_GPL(__irq_domain_add);
  */
 void irq_domain_remove(struct irq_domain *domain)
 {
-	mutex_lock(&irq_domain_mutex);
-	debugfs_remove_domain_dir(domain);
-
-	WARN_ON(!radix_tree_empty(&domain->revmap_tree));
-
-	list_del(&domain->link);
-
-	/*
-	 * If the going away domain is the default one, reset it.
-	 */
-	if (unlikely(irq_default_domain == domain))
-		irq_set_default_host(NULL);
-
-	mutex_unlock(&irq_domain_mutex);
-
-	pr_debug("Removed domain %s\n", domain->name);
-
-	fwnode_dev_initialized(domain->fwnode, false);
-	fwnode_handle_put(domain->fwnode);
-	if (domain->flags & IRQ_DOMAIN_NAME_ALLOCATED)
-		kfree(domain->name);
-	kfree(domain);
+	irq_domain_unpublish(domain);
+	irq_domain_free(domain);
 }
 EXPORT_SYMBOL_GPL(irq_domain_remove);
 
@@ -1183,7 +1224,7 @@ struct irq_domain *irq_domain_create_hierarchy(struct irq_domain *parent,
 		domain->parent = parent;
 		domain->flags |= flags;
 
-		__irq_domain_publish(domain);
+		irq_domain_publish(domain);
 	}
 
 	return domain;
-- 
2.44.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related

* [PATCH 10/17] irqdomain: Add missing parameter descriptions in docs
From: Herve Codina @ 2024-04-30  8:37 UTC (permalink / raw)
  To: Herve Codina, Thomas Gleixner, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Lee Jones, Arnd Bergmann, Horatiu Vultur,
	UNGLinuxDriver, Andrew Lunn, Heiner Kallweit, Russell King,
	Saravana Kannan, Bjorn Helgaas, Philipp Zabel, Lars Povlsen,
	Steen Hegelund, Daniel Machon, Alexandre Belloni
  Cc: linux-kernel, devicetree, netdev, linux-pci, linux-arm-kernel,
	Allan Nielsen, Steen Hegelund, Luca Ceresoli, Thomas Petazzoni
In-Reply-To: <20240430083730.134918-1-herve.codina@bootlin.com>

During compilation, several warning of the following form were raised:
  Function parameter or struct member 'x' not described in 'yyy'

Add the missing function parameter descriptions.

Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
 kernel/irq/irqdomain.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index 3dd1c871e091..1ed8c4d3cf2e 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -111,6 +111,7 @@ EXPORT_SYMBOL_GPL(__irq_domain_alloc_fwnode);
 
 /**
  * irq_domain_free_fwnode - Free a non-OF-backed fwnode_handle
+ * @fwnode: fwnode_handle to free
  *
  * Free a fwnode_handle allocated with irq_domain_alloc_fwnode.
  */
@@ -981,6 +982,12 @@ EXPORT_SYMBOL_GPL(__irq_resolve_mapping);
 
 /**
  * irq_domain_xlate_onecell() - Generic xlate for direct one cell bindings
+ * @d: IRQ domain involved in the translation
+ * @ctrlr: the DT node for the device whose interrupt we're translating
+ * @intspec: the interrupt specifier data from the DT
+ * @intsize: the number of entries in @intspec
+ * @out_hwirq: pointer at which to write the hwirq number
+ * @out_type: pointer at which to write the interrupt type
  *
  * Device Tree IRQ specifier translation function which works with one cell
  * bindings where the cell value maps directly to the hwirq number.
@@ -999,6 +1006,12 @@ EXPORT_SYMBOL_GPL(irq_domain_xlate_onecell);
 
 /**
  * irq_domain_xlate_twocell() - Generic xlate for direct two cell bindings
+ * @d: IRQ domain involved in the translation
+ * @ctrlr: the DT node for the device whose interrupt we're translating
+ * @intspec: the interrupt specifier data from the DT
+ * @intsize: the number of entries in @intspec
+ * @out_hwirq: pointer at which to write the hwirq number
+ * @out_type: pointer at which to write the interrupt type
  *
  * Device Tree IRQ specifier translation function which works with two cell
  * bindings where the cell values map directly to the hwirq number
@@ -1017,6 +1030,12 @@ EXPORT_SYMBOL_GPL(irq_domain_xlate_twocell);
 
 /**
  * irq_domain_xlate_onetwocell() - Generic xlate for one or two cell bindings
+ * @d: IRQ domain involved in the translation
+ * @ctrlr: the DT node for the device whose interrupt we're translating
+ * @intspec: the interrupt specifier data from the DT
+ * @intsize: the number of entries in @intspec
+ * @out_hwirq: pointer at which to write the hwirq number
+ * @out_type: pointer at which to write the interrupt type
  *
  * Device Tree IRQ specifier translation function which works with either one
  * or two cell bindings where the cell values map directly to the hwirq number
@@ -1050,6 +1069,10 @@ EXPORT_SYMBOL_GPL(irq_domain_simple_ops);
 /**
  * irq_domain_translate_onecell() - Generic translate for direct one cell
  * bindings
+ * @d: IRQ domain involved in the translation
+ * @fwspec: FW interrupt specifier to translate
+ * @out_hwirq: pointer at which to write the hwirq number
+ * @out_type: pointer at which to write the interrupt type
  */
 int irq_domain_translate_onecell(struct irq_domain *d,
 				 struct irq_fwspec *fwspec,
@@ -1067,6 +1090,10 @@ EXPORT_SYMBOL_GPL(irq_domain_translate_onecell);
 /**
  * irq_domain_translate_twocell() - Generic translate for direct two cell
  * bindings
+ * @d: IRQ domain involved in the translation
+ * @fwspec: FW interrupt specifier to translate
+ * @out_hwirq: pointer at which to write the hwirq number
+ * @out_type: pointer at which to write the interrupt type
  *
  * Device Tree IRQ specifier translation function which works with two cell
  * bindings where the cell values map directly to the hwirq number
-- 
2.44.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related

* [PATCH 09/17] dt-bindings: interrupt-controller: Add support for Microchip LAN966x OIC
From: Herve Codina @ 2024-04-30  8:37 UTC (permalink / raw)
  To: Herve Codina, Thomas Gleixner, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Lee Jones, Arnd Bergmann, Horatiu Vultur,
	UNGLinuxDriver, Andrew Lunn, Heiner Kallweit, Russell King,
	Saravana Kannan, Bjorn Helgaas, Philipp Zabel, Lars Povlsen,
	Steen Hegelund, Daniel Machon, Alexandre Belloni
  Cc: linux-kernel, devicetree, netdev, linux-pci, linux-arm-kernel,
	Allan Nielsen, Steen Hegelund, Luca Ceresoli, Thomas Petazzoni
In-Reply-To: <20240430083730.134918-1-herve.codina@bootlin.com>

The Microchip LAN966x outband interrupt controller (OIC) maps the
internal interrupt sources of the LAN966x device to an external
interrupt.
When the LAN966x device is used as a PCI device, the external interrupt
is routed to the PCI interrupt.

Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
 .../microchip,lan966x-oic.yaml                | 55 +++++++++++++++++++
 1 file changed, 55 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/microchip,lan966x-oic.yaml

diff --git a/Documentation/devicetree/bindings/interrupt-controller/microchip,lan966x-oic.yaml b/Documentation/devicetree/bindings/interrupt-controller/microchip,lan966x-oic.yaml
new file mode 100644
index 000000000000..b2adc7174177
--- /dev/null
+++ b/Documentation/devicetree/bindings/interrupt-controller/microchip,lan966x-oic.yaml
@@ -0,0 +1,55 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/interrupt-controller/microchip,lan966x-oic.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Microchip LAN966x outband interrupt controller
+
+maintainers:
+  - Herve Codina <herve.codina@bootlin.com>
+
+allOf:
+  - $ref: /schemas/interrupt-controller.yaml#
+
+description: |
+  The Microchip LAN966x outband interrupt controller (OIC) maps the internal
+  interrupt sources of the LAN966x device to an external interrupt.
+  When the LAN966x device is used as a PCI device, the external interrupt is
+  routed to the PCI interrupt.
+
+properties:
+  compatible:
+    const: microchip,lan966x-oic
+
+  '#interrupt-cells':
+    const: 2
+
+  interrupt-controller: true
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+required:
+  - compatible
+  - '#interrupt-cells'
+  - interrupt-controller
+  - interrupts
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    interrupt-controller@e00c0120 {
+        compatible = "microchip,lan966x-oic";
+        reg = <0xe00c0120 0x190>;
+        #interrupt-cells = <2>;
+        interrupt-controller;
+        interrupts = <0>;
+        interrupt-parent = <&intc>;
+    };
+...
-- 
2.44.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related

* [PATCH 08/17] net: lan966x: remove debugfs directory in probe() error path
From: Herve Codina @ 2024-04-30  8:37 UTC (permalink / raw)
  To: Herve Codina, Thomas Gleixner, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Lee Jones, Arnd Bergmann, Horatiu Vultur,
	UNGLinuxDriver, Andrew Lunn, Heiner Kallweit, Russell King,
	Saravana Kannan, Bjorn Helgaas, Philipp Zabel, Lars Povlsen,
	Steen Hegelund, Daniel Machon, Alexandre Belloni
  Cc: linux-kernel, devicetree, netdev, linux-pci, linux-arm-kernel,
	Allan Nielsen, Steen Hegelund, Luca Ceresoli, Thomas Petazzoni,
	stable
In-Reply-To: <20240430083730.134918-1-herve.codina@bootlin.com>

A debugfs directory entry is create early during probe(). This entry is
not removed on error path leading to some "already present" issues in
case of EPROBE_DEFER.

Create this entry later in the probe() code to avoid the need to change
many 'return' in 'goto' and add the removal in the already present error
path.

Fixes: 942814840127 ("net: lan966x: Add VCAP debugFS support")
Cc: <stable@vger.kernel.org>
Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
 drivers/net/ethernet/microchip/lan966x/lan966x_main.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
index 2635ef8958c8..61d88207eed4 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
@@ -1087,8 +1087,6 @@ static int lan966x_probe(struct platform_device *pdev)
 	platform_set_drvdata(pdev, lan966x);
 	lan966x->dev = &pdev->dev;
 
-	lan966x->debugfs_root = debugfs_create_dir("lan966x", NULL);
-
 	if (!device_get_mac_address(&pdev->dev, mac_addr)) {
 		ether_addr_copy(lan966x->base_mac, mac_addr);
 	} else {
@@ -1179,6 +1177,8 @@ static int lan966x_probe(struct platform_device *pdev)
 		return dev_err_probe(&pdev->dev, -ENODEV,
 				     "no ethernet-ports child found\n");
 
+	lan966x->debugfs_root = debugfs_create_dir("lan966x", NULL);
+
 	/* init switch */
 	lan966x_init(lan966x);
 	lan966x_stats_init(lan966x);
@@ -1257,6 +1257,8 @@ static int lan966x_probe(struct platform_device *pdev)
 	destroy_workqueue(lan966x->stats_queue);
 	mutex_destroy(&lan966x->stats_lock);
 
+	debugfs_remove_recursive(lan966x->debugfs_root);
+
 	return err;
 }
 
-- 
2.44.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related

* [PATCH 07/17] net: mdio: mscc-miim: Handle the switch reset
From: Herve Codina @ 2024-04-30  8:37 UTC (permalink / raw)
  To: Herve Codina, Thomas Gleixner, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Lee Jones, Arnd Bergmann, Horatiu Vultur,
	UNGLinuxDriver, Andrew Lunn, Heiner Kallweit, Russell King,
	Saravana Kannan, Bjorn Helgaas, Philipp Zabel, Lars Povlsen,
	Steen Hegelund, Daniel Machon, Alexandre Belloni
  Cc: linux-kernel, devicetree, netdev, linux-pci, linux-arm-kernel,
	Allan Nielsen, Steen Hegelund, Luca Ceresoli, Thomas Petazzoni
In-Reply-To: <20240430083730.134918-1-herve.codina@bootlin.com>

The mscc-miim device can be impacted by the switch reset, at least when
this device is part of the LAN966x PCI device.

Handle this newly added (optional) resets property.

Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
 drivers/net/mdio/mdio-mscc-miim.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/net/mdio/mdio-mscc-miim.c b/drivers/net/mdio/mdio-mscc-miim.c
index c29377c85307..6a6c1768f533 100644
--- a/drivers/net/mdio/mdio-mscc-miim.c
+++ b/drivers/net/mdio/mdio-mscc-miim.c
@@ -19,6 +19,7 @@
 #include <linux/platform_device.h>
 #include <linux/property.h>
 #include <linux/regmap.h>
+#include <linux/reset.h>
 
 #define MSCC_MIIM_REG_STATUS		0x0
 #define		MSCC_MIIM_STATUS_STAT_PENDING	BIT(2)
@@ -270,11 +271,18 @@ static int mscc_miim_probe(struct platform_device *pdev)
 {
 	struct device_node *np = pdev->dev.of_node;
 	struct regmap *mii_regmap, *phy_regmap;
+	struct reset_control *reset;
 	struct device *dev = &pdev->dev;
 	struct mscc_miim_dev *miim;
 	struct mii_bus *bus;
 	int ret;
 
+	reset = devm_reset_control_get_optional_shared(dev, "switch");
+	if (IS_ERR(reset))
+		return dev_err_probe(dev, PTR_ERR(reset), "Failed to get reset\n");
+
+	reset_control_reset(reset);
+
 	mii_regmap = ocelot_regmap_from_resource(pdev, 0,
 						 &mscc_miim_regmap_config);
 	if (IS_ERR(mii_regmap))
-- 
2.44.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related

* Re: [PATCH 0/6] pmdomain/cpuidle-psci: Support s2idle/s2ram on PREEMPT_RT
From: Sebastian Andrzej Siewior @ 2024-04-30  9:44 UTC (permalink / raw)
  To: Ulf Hansson
  Cc: Rafael J . Wysocki, Sudeep Holla, linux-pm, Lorenzo Pieralisi,
	Nikunj Kela, Prasad Sodagudi, Maulik Shah, Daniel Lezcano,
	Krzysztof Kozlowski, linux-rt-users, linux-arm-kernel,
	linux-kernel
In-Reply-To: <20240429140531.210576-1-ulf.hansson@linaro.org>

On 2024-04-29 16:05:25 [+0200], Ulf Hansson wrote:
> The hierarchical PM domain topology and the corresponding domain-idle-states
> are currently disabled on a PREEMPT_RT based configuration. The main reason is
> because spinlocks are turned into sleepable locks on PREEMPT_RT, which means
> genpd and runtime PM can't be use in the atomic idle-path when
> selecting/entering an idle-state.
> 
> For s2idle/s2ram this is an unnecessary limitation that this series intends to
> address. Note that, the support for cpuhotplug is left to future improvements.
> More information about this are available in the commit messages.
> 
> I have tested this on a Dragonboard 410c.

Have you tested this with PREEMPT_RT enabled and if so, which kernel?

> Kind regards
> Ulf Hansson

Sebastian

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* Re: [PATCH net-next v2] net: ti: icssg_prueth: Add SW TX / RX Coalescing based on hrtimers
From: MD Danish Anwar @ 2024-04-30  9:42 UTC (permalink / raw)
  To: Simon Horman
  Cc: Dan Carpenter, Heiner Kallweit, Andrew Lunn, Jan Kiszka,
	Diogo Ivo, Paolo Abeni, Jakub Kicinski, Eric Dumazet,
	David S. Miller, linux-kernel, netdev, linux-arm-kernel, srk,
	Vignesh Raghavendra, r-gunasekaran, Roger Quadros
In-Reply-To: <20240429183034.GG516117@kernel.org>

Simon,

On 30/04/24 12:00 am, Simon Horman wrote:
> On Mon, Apr 29, 2024 at 12:45:01PM +0530, MD Danish Anwar wrote:
>> Add SW IRQ coalescing based on hrtimers for RX and TX data path for ICSSG
>> driver, which can be enabled by ethtool commands:
>>
>> - RX coalescing
>>   ethtool -C eth1 rx-usecs 50
>>
>> - TX coalescing can be enabled per TX queue
>>
>>   - by default enables coalesing for TX0
> 
> nit: coalescing
> 
> Please consider running patches through ./checkpatch --codespell
> 
>>   ethtool -C eth1 tx-usecs 50
>>   - configure TX0
>>   ethtool -Q eth0 queue_mask 1 --coalesce tx-usecs 100
>>   - configure TX1
>>   ethtool -Q eth0 queue_mask 2 --coalesce tx-usecs 100
>>   - configure TX0 and TX1
>>   ethtool -Q eth0 queue_mask 3 --coalesce tx-usecs 100 --coalesce
>> tx-usecs 100
>>
>> Minimum value for both rx-usecs and tx-usecs is 20us.
>>
>> Compared to gro_flush_timeout and napi_defer_hard_irqs this patch allows
>> to enable IRQ coalescing for RX path separately.
>>
>> Benchmarking numbers:
>>  ===============================================================
>> | Method                  | Tput_TX | CPU_TX | Tput_RX | CPU_RX |
>> | ==============================================================
>> | Default Driver           943 Mbps    31%      517 Mbps  38%   |
>> | IRQ Coalescing (Patch)   943 Mbps    28%      518 Mbps  25%   |
>>  ===============================================================
>>
>> Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
>> ---

[ ... ]
>>  	if (num_tx_packets >= budget)
>>  		return budget;
>>  
>> -	if (napi_complete_done(napi_tx, num_tx_packets))
>> -		enable_irq(tx_chn->irq);
>> +	if (napi_complete_done(napi_tx, num_tx_packets)) {
>> +		if (unlikely(tx_chn->tx_pace_timeout_ns && !tdown)) {
>> +			hrtimer_start(&tx_chn->tx_hrtimer,
>> +				      ns_to_ktime(tx_chn->tx_pace_timeout_ns),
>> +				      HRTIMER_MODE_REL_PINNED);
>> +		} else {
>> +			enable_irq(tx_chn->irq);
>> +		}
> 
> This compiles with gcc-13 and clang-18 W=1
> (although the inner {} are unnecessary).
> 
>> +	}
>>  
>>  	return num_tx_packets;
>>  }
> 
> ...
> 
>> @@ -872,7 +894,13 @@ int emac_napi_rx_poll(struct napi_struct *napi_rx, int budget)
>>  	}
>>  
>>  	if (num_rx < budget && napi_complete_done(napi_rx, num_rx))
>> -		enable_irq(emac->rx_chns.irq[rx_flow]);
>> +		if (unlikely(emac->rx_pace_timeout_ns)) {
>> +			hrtimer_start(&emac->rx_hrtimer,
>> +				      ns_to_ktime(emac->rx_pace_timeout_ns),
>> +				      HRTIMER_MODE_REL_PINNED);
>> +		} else {
>> +			enable_irq(emac->rx_chns.irq[rx_flow]);
>> +		}
> 
> But this does not; I think outer (but not inner) {} are needed.
> 

For both of these if checks, by having {} for outer if I am not seeing
the warnings anymore. The braces don't seem to be neccessary for inner if.

For both of these ifs I'll keep both inner and outer ifs in braces as
this will help readablity as Dan pointed out.

I will post v3 with this change.

> FIIIW, I believe this doesn't show-up in the netdev automated testing
> because this driver isn't built for x86 allmodconfig.
> 
>>  
>>  	return num_rx;
>>  }
> 
> ...
> 

-- 
Thanks and Regards,
Danish

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* Re: [PATCH] mailbox: mtk-cmdq: Fix sleeping function called from invalid context
From: AngeloGioacchino Del Regno @ 2024-04-30  9:39 UTC (permalink / raw)
  To: Jason-JH.Lin, Jassi Brar, Chun-Kuang Hu, Matthias Brugger
  Cc: Jason-ch Chen, Singo Chang, Nancy Lin, Shawn Sung, linux-kernel,
	linux-arm-kernel, linux-mediatek,
	Project_Global_Chrome_Upstream_Group
In-Reply-To: <20240430083934.26980-1-jason-jh.lin@mediatek.com>

Il 30/04/24 10:39, Jason-JH.Lin ha scritto:
> When we run kernel with lockdebug option, we will get the BUG below:
> [  106.692124] BUG: sleeping function called from invalid context at drivers/base/power/runtime.c:1164
> [  106.692190] in_atomic(): 1, irqs_disabled(): 128, non_block: 0, pid: 3616, name: kworker/u17:3
> [  106.692226] preempt_count: 1, expected: 0
> [  106.692254] RCU nest depth: 0, expected: 0
> [  106.692282] INFO: lockdep is turned off.
> [  106.692306] irq event stamp: 0
> [  106.692331] hardirqs last  enabled at (0): [<0000000000000000>] 0x0
> [  106.692376] hardirqs last disabled at (0): [<ffffffee15d37fa0>] copy_process+0xc90/0x2ac0
> [  106.692429] softirqs last  enabled at (0): [<ffffffee15d37fc4>] copy_process+0xcb4/0x2ac0
> [  106.692473] softirqs last disabled at (0): [<0000000000000000>] 0x0
> [  106.692513] CPU: 1 PID: 3616 Comm: kworker/u17:3 Not tainted 6.1.87-lockdep-14133-g26e933aca785 #1 6839942e1cf34914b0a366137843dd2366f52aa9
> [  106.692556] Hardware name: Google Ciri sku0/unprovisioned board (DT)
> [  106.692586] Workqueue: imgsys_runner imgsys_runner_func
> [  106.692638] Call trace:
> [  106.692662]  dump_backtrace+0x100/0x120
> [  106.692702]  show_stack+0x20/0x2c
> [  106.692737]  dump_stack_lvl+0x84/0xb4
> [  106.692775]  dump_stack+0x18/0x48
> [  106.692809]  __might_resched+0x354/0x4c0
> [  106.692847]  __might_sleep+0x98/0xe4
> [  106.692883]  __pm_runtime_resume+0x70/0x124
> [  106.692921]  cmdq_mbox_send_data+0xe4/0xb1c
> [  106.692964]  msg_submit+0x194/0x2dc
> [  106.693003]  mbox_send_message+0x190/0x330
> [  106.693043]  imgsys_cmdq_sendtask+0x1618/0x2224
> [  106.693082]  imgsys_runner_func+0xac/0x11c
> [  106.693118]  process_one_work+0x638/0xf84
> [  106.693158]  worker_thread+0x808/0xcd0
> [  106.693196]  kthread+0x24c/0x324
> [  106.693231]  ret_from_fork+0x10/0x20
> 
> We found that there is a spin_lock_irqsave protection in msg_submit()
> of mailbox.c.
> So when cmdq driver calls pm_runtime_get_sync() in cmdq_mbox_send_data(),
> it will get this BUG report.
> 
> Add pm_runtime_irq_safe() to let pm_runtime callbacks is safe in atomic
> context with interrupts disabled.
> 

I see. The problem with this is that pm_runtime_irq_safe() will raise the refcount
of the parent device "forever", which isn't the best and partially defeats what we
are trying to do here (keeping stuff off unless really needed).

At this point I wonder if it'd be just better to really fix this instead of working
around the problem.

I'd say that one of the options would be to perform a change to msg_submit() so
that it will take into account that a .send_data() callback might be using RPM
functions.

Ideas? :-)

Cheers,
Angelo

> Fixes: 8afe816b0c99 ("mailbox: mtk-cmdq-mailbox: Implement Runtime PM with autosuspend")
> Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> ---
>   drivers/mailbox/mtk-cmdq-mailbox.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/mailbox/mtk-cmdq-mailbox.c b/drivers/mailbox/mtk-cmdq-mailbox.c
> index ead2200f39ba..3b4f19633c83 100644
> --- a/drivers/mailbox/mtk-cmdq-mailbox.c
> +++ b/drivers/mailbox/mtk-cmdq-mailbox.c
> @@ -694,6 +694,7 @@ static int cmdq_probe(struct platform_device *pdev)
>   
>   	pm_runtime_set_autosuspend_delay(dev, CMDQ_MBOX_AUTOSUSPEND_DELAY_MS);
>   	pm_runtime_use_autosuspend(dev);
> +	pm_runtime_irq_safe(dev);
>   
>   	return 0;
>   }




_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* Re: [PATCH] ARM: Use conditionals for CFI branches
From: Ard Biesheuvel @ 2024-04-30  9:37 UTC (permalink / raw)
  To: Russell King (Oracle)
  Cc: Linus Walleij, Sami Tolvanen, Kees Cook, Nathan Chancellor,
	Nick Desaulniers, Arnd Bergmann, linux-arm-kernel, llvm
In-Reply-To: <ZjC4iL7lAvReKbx0@shell.armlinux.org.uk>

On Tue, 30 Apr 2024 at 11:23, Russell King (Oracle)
<linux@armlinux.org.uk> wrote:
>
> On Tue, Apr 30, 2024 at 11:18:55AM +0200, Ard Biesheuvel wrote:
> > On Tue, 30 Apr 2024 at 10:26, Linus Walleij <linus.walleij@linaro.org> wrote:
> > > diff --git a/arch/arm/mm/cache-fa.S b/arch/arm/mm/cache-fa.S
> > > index db454033b76f..4a3668b52a2d 100644
> > > --- a/arch/arm/mm/cache-fa.S
> > > +++ b/arch/arm/mm/cache-fa.S
> > > @@ -112,7 +112,9 @@ SYM_FUNC_END(fa_flush_user_cache_range)
> > >   *     - end    - virtual end address
> > >   */
> > >  SYM_TYPED_FUNC_START(fa_coherent_kern_range)
> > > +#ifdef CONFIG_CFI_CLANG /* Fallthrough if !CFI */
> >
> > These functions are only called indirectly if MULTI_CACHE is enabled,
> > right? If so, this could be
> >
> > #if defined(CONFIG_CFI_CLANG) && defined(MULTI_CACHE)
>
> I don't see that makes any difference. Whether or not they're called
> indirectly, the symbol is the entry point to the function. If called
> directly and the useless branch is there, we'll incur the overhead of
> the BL instruction flushing the pipeline followed immediately by the
> overhead of the B instruction flushing the pipeline again.

That is not what I meant.

I meant that, if you decide to enable CFI clang for these targets, you
still only need the branch if MULTI_CACHE is enabled.

IOW, we can avoid the branch entirely in even more situations.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* Re: [PATCH] arm64: dts: mediatek: mt8192-asurada: Add off-on-delay-us for pp3300_mipibrdg
From: Pin-yen Lin @ 2024-04-30  9:32 UTC (permalink / raw)
  To: AngeloGioacchino Del Regno
  Cc: Matthias Brugger,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	moderated list:ARM/Mediatek SoC support,
	moderated list:ARM/Mediatek SoC support,
	Nícolas F . R . A . Prado,
	open list:ARM/Mediatek SoC support, Hsin-Te Yuan
In-Reply-To: <b3c69a78-78c9-4a15-829b-e4b36e16566a@collabora.com>

Hi Angelo,

On Tue, Apr 30, 2024 at 4:17 PM AngeloGioacchino Del Regno
<angelogioacchino.delregno@collabora.com> wrote:
>
> Il 29/04/24 11:53, Pin-yen Lin ha scritto:
> > Set off-on-delay-us to 500000 us for pp3300_mipibrdg to make sure it
> > complies with the panel sequence. Explicit configuration on the
> > regulator node is required because mt8192-asurada uses the same power
> > supply for the panel and the anx7625 DP bridge. So powering on/off the
> > DP bridge could break the power sequence requirement for the panel.
> >
> > Fixes: f9f00b1f6b9b ("arm64: dts: mediatek: asurada: Add display regulators")
> > Signed-off-by: Pin-yen Lin <treapking@chromium.org>
> >
>
> Uhm, there might be more to it - I don't think that this should ever happen.
>
> The regulator is refcounted, so...
>   * Bridge on: panel goes off, but regulator doesn't turn off (refcount=1)
>     * Panel resume -> sequence respected (refcount=2 -> wait -> more vregs, etc)
>   * Bridge off: panel is already off (refcount=0)
>     * Bridge resume -> refcount=1, no panel commands yet

The off-on-delay could be violated because the bridge driver does not
check the delay.

>     * Panel resume -> refcount=2, wait -> more vregs, etc
>
> Can you please describe the issue that you're getting?

The symptom we observed is that the device has a small chance to
reboot to a black panel, and we think the panel's unprepare delay (the
time to power down completely) might not be satisfied because the
bridge doesn't check that when it enables the regulator. Even if the
regulator is enabled by the panel driver, the delay can also be
violated in the following sequence:

* t=0ms, bridge on: panel goes off, but regulator doesn't turn off
(refcount=1). The .unprepared_time in panel_edp is updated
* t=300ms, bridge off, regulator goes off (refcount=0)
* t=600ms, panel on, the panel driver thinks the unprepare delay
(500ms) is satisfied, but the regulator was disabled 300ms ago.

Did I miss anything here? Or should I add more detail to the commit message?

>
> Cheers,
> Angelo
>
Regards,
Pin-yen

> > ---
> >
> >   arch/arm64/boot/dts/mediatek/mt8192-asurada.dtsi | 1 +
> >   1 file changed, 1 insertion(+)
> >
> > diff --git a/arch/arm64/boot/dts/mediatek/mt8192-asurada.dtsi b/arch/arm64/boot/dts/mediatek/mt8192-asurada.dtsi
> > index 7a704246678f..08d71ddf3668 100644
> > --- a/arch/arm64/boot/dts/mediatek/mt8192-asurada.dtsi
> > +++ b/arch/arm64/boot/dts/mediatek/mt8192-asurada.dtsi
> > @@ -147,6 +147,7 @@ pp3300_mipibrdg: regulator-3v3-mipibrdg {
> >               regulator-boot-on;
> >               gpio = <&pio 127 GPIO_ACTIVE_HIGH>;
> >               vin-supply = <&pp3300_g>;
> > +             off-on-delay-us = <500000>;
> >       };
> >
> >       /* separately switched 3.3V power rail */
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* Re: [PATCH v8 04/16] ACPI: processor: Move checks and availability of acpi_processor earlier
From: Jonathan Cameron @ 2024-04-30  9:28 UTC (permalink / raw)
  To: Gavin Shan
  Cc: Thomas Gleixner, Peter Zijlstra, linux-pm, loongarch, linux-acpi,
	linux-arch, linux-kernel, linux-arm-kernel, kvmarm, x86,
	Russell King, Rafael J . Wysocki, Miguel Luis, James Morse,
	Salil Mehta, Jean-Philippe Brucker, Catalin Marinas, Will Deacon,
	Marc Zyngier, Hanjun Guo, Ingo Molnar, Borislav Petkov,
	Dave Hansen, linuxarm, justin.he, jianyong.wu, Lorenzo Pieralisi,
	Sudeep Holla
In-Reply-To: <80a2e07f-ecb2-48af-b2be-646f17e0e63e@redhat.com>

On Tue, 30 Apr 2024 14:17:24 +1000
Gavin Shan <gshan@redhat.com> wrote:

> On 4/26/24 23:51, Jonathan Cameron wrote:
> > Make the per_cpu(processors, cpu) entries available earlier so that
> > they are available in arch_register_cpu() as ARM64 will need access
> > to the acpi_handle to distinguish between acpi_processor_add()
> > and earlier registration attempts (which will fail as _STA cannot
> > be checked).
> > 
> > Reorder the remove flow to clear this per_cpu() after
> > arch_unregister_cpu() has completed, allowing it to be used in
> > there as well.
> > 
> > Note that on x86 for the CPU hotplug case, the pr->id prior to
> > acpi_map_cpu() may be invalid. Thus the per_cpu() structures
> > must be initialized after that call or after checking the ID
> > is valid (not hotplug path).
> > 
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > 
> > ---
> > v8: On buggy bios detection when setting per_cpu structures
> >      do not carry on.
> >      Fix up the clearing of per cpu structures to remove unwanted
> >      side effects and ensure an error code isn't use to reference them.
> > ---
> >   drivers/acpi/acpi_processor.c | 79 +++++++++++++++++++++--------------
> >   1 file changed, 48 insertions(+), 31 deletions(-)
> > 
> > diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c
> > index ba0a6f0ac841..3b180e21f325 100644
> > --- a/drivers/acpi/acpi_processor.c
> > +++ b/drivers/acpi/acpi_processor.c
> > @@ -183,8 +183,38 @@ static void __init acpi_pcc_cpufreq_init(void) {}
> >   #endif /* CONFIG_X86 */
> >   
> >   /* Initialization */
> > +static DEFINE_PER_CPU(void *, processor_device_array);
> > +
> > +static bool acpi_processor_set_per_cpu(struct acpi_processor *pr,
> > +				       struct acpi_device *device)
> > +{
> > +	BUG_ON(pr->id >= nr_cpu_ids);  
> 
> One blank line after BUG_ON() if we need to follow original implementation.

Sure unintentional - I'll put that back.

> 
> > +	/*
> > +	 * Buggy BIOS check.
> > +	 * ACPI id of processors can be reported wrongly by the BIOS.
> > +	 * Don't trust it blindly
> > +	 */
> > +	if (per_cpu(processor_device_array, pr->id) != NULL &&
> > +	    per_cpu(processor_device_array, pr->id) != device) {
> > +		dev_warn(&device->dev,
> > +			 "BIOS reported wrong ACPI id %d for the processor\n",
> > +			 pr->id);
> > +		/* Give up, but do not abort the namespace scan. */  
> 
> It depends on how the return value is handled by the caller if the namespace
> is continued to be scanned. The caller can be acpi_processor_hotadd_init()
> and acpi_processor_get_info() after this patch is applied. So I think this
> specific comment need to be moved to the caller.

Good point. This gets messy and was an unintended change.

Previously the options were:
1) acpi_processor_get_info() failed for other reasons - this code was never called.
2) acpi_processor_get_info() succeeded without acpi_processor_hotadd_init (non hotplug)
   this code then ran and would paper over the problem doing a bunch of cleanup under err.
3) acpi_processor_get_info() succeeded with acpi_processor_hotadd_init called.
   This code then ran and would paper over the problem doing a bunch of cleanup under err.

We should maintain that or argue cleanly against it.

This isn't helped the the fact I have no idea which cases we care about for that bios
bug handling.  Do any of those bios's ever do hotplug?  Guess we have to try and maintain
whatever protection this was offering.

Also, the original code leaks data in some paths and I have limited idea
of whether it is intentional or not. So to tidy the issue up that you've identified
I'll need to try and make that code consistent first.

I suspect the only way to do that is going to be to duplicate the allocations we
'want' to leak to deal with the bios bug detection.

For example acpi_processor_get_info() failing leaks pr and pr->throttling.shared_cpu_map
before this series. After this series we need pr to leak because it's used for the detection
via processor_device_array.

I'll work through this but it's going to be tricky to tell if we get right.
Step 1 will be closing the existing leaks and then we will have something
consistent to build on.

> 
> Besides, it seems acpi_processor_set_per_cpu() isn't properly called and
> memory leakage can happen. More details are given below.
> 
> > +		return false;
> > +	}
> > +	/*
> > +	 * processor_device_array is not cleared on errors to allow buggy BIOS
> > +	 * checks.
> > +	 */
> > +	per_cpu(processor_device_array, pr->id) = device;
> > +	per_cpu(processors, pr->id) = pr;
> > +
> > +	return true;
> > +}
> > +
> >   #ifdef CONFIG_ACPI_HOTPLUG_CPU
> > -static int acpi_processor_hotadd_init(struct acpi_processor *pr)
> > +static int acpi_processor_hotadd_init(struct acpi_processor *pr,
> > +				      struct acpi_device *device)
> >   {
> >   	int ret;
> >   
> > @@ -198,8 +228,15 @@ static int acpi_processor_hotadd_init(struct acpi_processor *pr)
> >   	if (ret)
> >   		goto out;
> >   
> > +	if (!acpi_processor_set_per_cpu(pr, device)) {
> > +		acpi_unmap_cpu(pr->id);
> > +		goto out;
> > +	}
> > +  
> 
> With the 'goto out', zero is returned from acpi_processor_hotadd_init() to acpi_processor_get_info().
> The zero return value is carried from acpi_map_cpu() in acpi_processor_hotadd_init(). If I'm correct,
> we need return errno from acpi_processor_get_info() to acpi_processor_add() so that cleanup can be
> done. For example, the cleanup corresponding to the 'err' tag can be done in acpi_processor_add().
> Otherwise, we will have memory leakage.
> 
> >   	ret = arch_register_cpu(pr->id);
> >   	if (ret) {
> > +		/* Leave the processor device array in place to detect buggy bios */
> > +		per_cpu(processors, pr->id) = NULL;
> >   		acpi_unmap_cpu(pr->id);
> >   		goto out;
> >   	}
> > @@ -217,7 +254,8 @@ static int acpi_processor_hotadd_init(struct acpi_processor *pr)
> >   	return ret;
> >   }
> >   #else
> > -static inline int acpi_processor_hotadd_init(struct acpi_processor *pr)
> > +static inline int acpi_processor_hotadd_init(struct acpi_processor *pr,
> > +					     struct acpi_device *device)
> >   {
> >   	return -ENODEV;
> >   }
> > @@ -316,10 +354,13 @@ static int acpi_processor_get_info(struct acpi_device *device)
> >   	 *  because cpuid <-> apicid mapping is persistent now.
> >   	 */
> >   	if (invalid_logical_cpuid(pr->id) || !cpu_present(pr->id)) {
> > -		int ret = acpi_processor_hotadd_init(pr);
> > +		int ret = acpi_processor_hotadd_init(pr, device);
> >   
> >   		if (ret)
> >   			return ret;
> > +	} else {
> > +		if (!acpi_processor_set_per_cpu(pr, device))
> > +			return 0;
> >   	}
> >     
> 
> For non-hotplug case, we still need pass the error to acpi_processor_add() so that
> cleanup corresponding 'err' tag can be done. Otherwise, we will have memory leakage.
> 
> >   	/*
> > @@ -365,8 +406,6 @@ static int acpi_processor_get_info(struct acpi_device *device)
> >    * (cpu_data(cpu)) values, like CPU feature flags, family, model, etc.
> >    * Such things have to be put in and set up by the processor driver's .probe().
> >    */
> > -static DEFINE_PER_CPU(void *, processor_device_array);
> > -
> >   static int acpi_processor_add(struct acpi_device *device,
> >   					const struct acpi_device_id *id)
> >   {
> > @@ -395,28 +434,6 @@ static int acpi_processor_add(struct acpi_device *device,
> >   	if (result) /* Processor is not physically present or unavailable */
> >   		return 0;
> >   
> > -	BUG_ON(pr->id >= nr_cpu_ids);
> > -
> > -	/*
> > -	 * Buggy BIOS check.
> > -	 * ACPI id of processors can be reported wrongly by the BIOS.
> > -	 * Don't trust it blindly
> > -	 */
> > -	if (per_cpu(processor_device_array, pr->id) != NULL &&
> > -	    per_cpu(processor_device_array, pr->id) != device) {
> > -		dev_warn(&device->dev,
> > -			"BIOS reported wrong ACPI id %d for the processor\n",
> > -			pr->id);
> > -		/* Give up, but do not abort the namespace scan. */
> > -		goto err;
> > -	}
> > -	/*
> > -	 * processor_device_array is not cleared on errors to allow buggy BIOS
> > -	 * checks.
> > -	 */
> > -	per_cpu(processor_device_array, pr->id) = device;
> > -	per_cpu(processors, pr->id) = pr;
> > -
> >   	dev = get_cpu_device(pr->id);
> >   	if (!dev) {
> >   		result = -ENODEV;
> > @@ -469,10 +486,6 @@ static void acpi_processor_remove(struct acpi_device *device)
> >   	device_release_driver(pr->dev);
> >   	acpi_unbind_one(pr->dev);
> >   
> > -	/* Clean up. */
> > -	per_cpu(processor_device_array, pr->id) = NULL;
> > -	per_cpu(processors, pr->id) = NULL;
> > -
> >   	cpu_maps_update_begin();
> >   	cpus_write_lock();
> >   
> > @@ -480,6 +493,10 @@ static void acpi_processor_remove(struct acpi_device *device)
> >   	arch_unregister_cpu(pr->id);
> >   	acpi_unmap_cpu(pr->id);
> >   
> > +	/* Clean up. */
> > +	per_cpu(processor_device_array, pr->id) = NULL;
> > +	per_cpu(processors, pr->id) = NULL;
> > +
> >   	cpus_write_unlock();
> >   	cpu_maps_update_done();
> >     
> 
> Thanks,
> Gavin
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox