Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1627819 > unrolled thread

Re: [PATCH V8 5/6] ACPI: Support the probing on the devices which apply indirect-IO

Started bydann frazier <dann.frazier@canonical.com>
First post2017-04-20 23:00 +0200
Last post2017-04-21 20:20 +0200
Articles 3 — 3 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH V8 5/6] ACPI: Support the probing on the devices which  apply indirect-IO dann frazier <dann.frazier@canonical.com> - 2017-04-20 23:00 +0200
    Re: [PATCH V8 5/6] ACPI: Support the probing on the devices which  apply indirect-IO "zhichang.yuan" <zhichang.yuan02@gmail.com> - 2017-04-21 04:30 +0200
      Re: [PATCH V8 5/6] ACPI: Support the probing on the devices which  apply indirect-IO Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> - 2017-04-21 20:20 +0200

#1627819 — Re: [PATCH V8 5/6] ACPI: Support the probing on the devices which apply indirect-IO

Fromdann frazier <dann.frazier@canonical.com>
Date2017-04-20 23:00 +0200
SubjectRe: [PATCH V8 5/6] ACPI: Support the probing on the devices which apply indirect-IO
Message-ID<tyrlf-5cz-7@gated-at.bofh.it>
On Thu, Mar 30, 2017 at 9:26 AM, zhichang.yuan
<yuanzhichang@hisilicon.com> wrote:
> On some platforms(such as Hip06/Hip07), the legacy ISA/LPC devices access I/O
> with some special host-local I/O ports known on x86. To access the I/O
> peripherals, an indirect-IO mechanism is introduced to mapped the host-local
> I/O to system logical/fake PIO similar the PCI MMIO on architectures where no
> separate I/O space exists. Just as PCI MMIO, the host I/O range should be
> registered before probing the downstream devices and set up the I/O mapping.
> But current ACPI bus probing doesn't support these indirect-IO hosts/devices.
>
> This patch introdueces a new ACPI handler for this device category. Through the
> handler attach callback, the indirect-IO hosts I/O registration is done and
> all peripherals' I/O resources are translated into logic/fake PIO before
> starting the enumeration.
>
> Signed-off-by: zhichang.yuan <yuanzhichang@hisilicon.com>
> Signed-off-by: Gabriele Paoloni <gabriele.paoloni@huawei.com>
> ---
>  drivers/acpi/Makefile          |   1 +
>  drivers/acpi/acpi_indirectio.c | 344 +++++++++++++++++++++++++++++++++++++++++
>  drivers/acpi/internal.h        |   5 +
>  drivers/acpi/scan.c            |   1 +
>  4 files changed, 351 insertions(+)
>  create mode 100644 drivers/acpi/acpi_indirectio.c
>
> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
> index a391bbc..10e5f2b 100644
> --- a/drivers/acpi/Makefile
> +++ b/drivers/acpi/Makefile
> @@ -57,6 +57,7 @@ acpi-$(CONFIG_ACPI_PROCFS_POWER) += cm_sbs.o
>  acpi-y                         += acpi_lpat.o
>  acpi-$(CONFIG_ACPI_GENERIC_GSI) += irq.o
>  acpi-$(CONFIG_ACPI_WATCHDOG)   += acpi_watchdog.o
> +acpi-$(CONFIG_INDIRECT_PIO)    += acpi_indirectio.o
>
>  # These are (potentially) separate modules
>
> diff --git a/drivers/acpi/acpi_indirectio.c b/drivers/acpi/acpi_indirectio.c
> new file mode 100644
> index 0000000..c8c80b5
> --- /dev/null
> +++ b/drivers/acpi/acpi_indirectio.c
> @@ -0,0 +1,344 @@
> +/*
> + * ACPI support for indirect-IO bus.
> + *
> + * Copyright (C) 2017 Hisilicon Limited, All Rights Reserved.
> + * Author: Zhichang Yuan <yuanzhichang@hisilicon.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/platform_device.h>
> +#include <linux/logic_pio.h>
> +
> +#include "internal.h"
> +
> +ACPI_MODULE_NAME("indirect IO");
> +
> +#define INDIRECT_IO_INFO(desc) ((unsigned long)&desc)
> +
> +struct lpc_private_data {
> +       resource_size_t io_size;
> +       resource_size_t io_start;
> +};
> +
> +struct indirectio_device_desc {
> +       void *pdata; /* device relevant info data */
> +       int (*pre_setup)(struct acpi_device *adev, void *pdata);
> +};
> +
> +static struct lpc_private_data lpc_data = {
> +       .io_size = LPC_BUS_IO_SIZE,
> +       .io_start = LPC_MIN_BUS_RANGE,
> +};
> +
> +static inline bool acpi_logicio_supported_resource(struct acpi_resource *res)
> +{
> +       switch (res->type) {
> +       case ACPI_RESOURCE_TYPE_ADDRESS32:
> +       case ACPI_RESOURCE_TYPE_ADDRESS64:
> +               return true;
> +       }
> +       return false;
> +}
> +
> +static acpi_status acpi_count_logiciores(struct acpi_resource *res,
> +                                          void *data)
> +{
> +       int *res_cnt = data;
> +
> +       if (acpi_logicio_supported_resource(res) &&
> +               !acpi_dev_filter_resource_type(res, IORESOURCE_IO))
> +               (*res_cnt)++;
> +
> +       return AE_OK;
> +}
> +
> +static acpi_status acpi_read_one_logiciores(struct acpi_resource *res,
> +               void *data)
> +{
> +       struct acpi_resource **resource = data;
> +
> +       if (acpi_logicio_supported_resource(res) &&
> +               !acpi_dev_filter_resource_type(res, IORESOURCE_IO)) {
> +               memcpy((*resource), res, sizeof(struct acpi_resource));
> +               (*resource)->length = sizeof(struct acpi_resource);
> +               (*resource)->type = res->type;
> +               (*resource)++;
> +       }
> +
> +       return AE_OK;
> +}
> +
> +static acpi_status
> +acpi_build_logiciores_template(struct acpi_device *adev,
> +                       struct acpi_buffer *buffer)
> +{
> +       acpi_handle handle = adev->handle;
> +       struct acpi_resource *resource;
> +       acpi_status status;
> +       int res_cnt = 0;
> +
> +       status = acpi_walk_resources(handle, METHOD_NAME__CRS,
> +                                    acpi_count_logiciores, &res_cnt);
> +       if (ACPI_FAILURE(status) || !res_cnt) {
> +               dev_err(&adev->dev, "can't evaluate _CRS: %d\n", status);
> +               return -EINVAL;
> +       }
> +
> +       buffer->length = sizeof(struct acpi_resource) * (res_cnt + 1) + 1;
> +       buffer->pointer = kzalloc(buffer->length - 1, GFP_KERNEL);

(Seth Forshee noticed this issue, just passing it on)

Should this just allocate the full buffer->length? That would keep the
length attribute accurate (possibly avoiding an off-by-1 error later).
It's not clear what the trailing byte is needed for, but other drivers
allocate it as well (drivers/acpi/pci_link.c and
drivers/platform/x86/sony-laptop.c).

 -dann

[toc] | [next] | [standalone]

#1627924

From"zhichang.yuan" <zhichang.yuan02@gmail.com>
Date2017-04-21 04:30 +0200
Message-ID<tywuC-7t-5@gated-at.bofh.it>
In reply to#1627819
Hi, Dann,



On 04/21/2017 04:57 AM, dann frazier wrote:
> On Thu, Mar 30, 2017 at 9:26 AM, zhichang.yuan
> <yuanzhichang@hisilicon.com> wrote:
>> On some platforms(such as Hip06/Hip07), the legacy ISA/LPC devices access I/O
>> with some special host-local I/O ports known on x86. To access the I/O
>> peripherals, an indirect-IO mechanism is introduced to mapped the host-local
>> I/O to system logical/fake PIO similar the PCI MMIO on architectures where no
>> separate I/O space exists. Just as PCI MMIO, the host I/O range should be
>> registered before probing the downstream devices and set up the I/O mapping.
>> But current ACPI bus probing doesn't support these indirect-IO hosts/devices.
>>
>> This patch introdueces a new ACPI handler for this device category. Through the
>> handler attach callback, the indirect-IO hosts I/O registration is done and
>> all peripherals' I/O resources are translated into logic/fake PIO before
>> starting the enumeration.
>>
>> Signed-off-by: zhichang.yuan <yuanzhichang@hisilicon.com>
>> Signed-off-by: Gabriele Paoloni <gabriele.paoloni@huawei.com>
>> ---
>>  drivers/acpi/Makefile          |   1 +
>>  drivers/acpi/acpi_indirectio.c | 344 +++++++++++++++++++++++++++++++++++++++++
>>  drivers/acpi/internal.h        |   5 +
>>  drivers/acpi/scan.c            |   1 +
>>  4 files changed, 351 insertions(+)
>>  create mode 100644 drivers/acpi/acpi_indirectio.c
>>
>> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
>> index a391bbc..10e5f2b 100644
>> --- a/drivers/acpi/Makefile
>> +++ b/drivers/acpi/Makefile
>> @@ -57,6 +57,7 @@ acpi-$(CONFIG_ACPI_PROCFS_POWER) += cm_sbs.o
>>  acpi-y                         += acpi_lpat.o
>>  acpi-$(CONFIG_ACPI_GENERIC_GSI) += irq.o
>>  acpi-$(CONFIG_ACPI_WATCHDOG)   += acpi_watchdog.o
>> +acpi-$(CONFIG_INDIRECT_PIO)    += acpi_indirectio.o
>>
>>  # These are (potentially) separate modules
>>
>> diff --git a/drivers/acpi/acpi_indirectio.c b/drivers/acpi/acpi_indirectio.c
>> new file mode 100644
>> index 0000000..c8c80b5
>> --- /dev/null
>> +++ b/drivers/acpi/acpi_indirectio.c
>> @@ -0,0 +1,344 @@

[snip]

>> +acpi_build_logiciores_template(struct acpi_device *adev,
>> +                       struct acpi_buffer *buffer)
>> +{
>> +       acpi_handle handle = adev->handle;
>> +       struct acpi_resource *resource;
>> +       acpi_status status;
>> +       int res_cnt = 0;
>> +
>> +       status = acpi_walk_resources(handle, METHOD_NAME__CRS,
>> +                                    acpi_count_logiciores, &res_cnt);
>> +       if (ACPI_FAILURE(status) || !res_cnt) {
>> +               dev_err(&adev->dev, "can't evaluate _CRS: %d\n", status);
>> +               return -EINVAL;
>> +       }
>> +
>> +       buffer->length = sizeof(struct acpi_resource) * (res_cnt + 1) + 1;
>> +       buffer->pointer = kzalloc(buffer->length - 1, GFP_KERNEL);
> 
> (Seth Forshee noticed this issue, just passing it on)
> 
> Should this just allocate the full buffer->length? That would keep the
> length attribute accurate (possibly avoiding an off-by-1 error later).
> It's not clear what the trailing byte is needed for, but other drivers
> allocate it as well (drivers/acpi/pci_link.c and
> drivers/platform/x86/sony-laptop.c).

Thanks for your suggestion!

I also curious why this one appended byte is needed as it seems the later
acpi_set_current_resources() doesn't use this byte.
And I tested without setting the buffer->length as the length of resource list
directly, it seems ok.

But anyway, it looks more reasonable to allocate the memory with the
buffer->length rather than buffer->length - 1;

I was made the V9 patch-set, and I can add your suggestion there. But I also
awaiting for ARM64 ACPI maintainer's comment about this patch before really
sending V9. I wonder whether there is better way to make our indirect-IO devices
can be assigned the logic PIO before the enumeration...

Lorenzo, Hanjun, what do you think about this patch?

Thanks,
Zhichang

> 
>  -dann
>

[toc] | [prev] | [next] | [standalone]

#1628411

FromLorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Date2017-04-21 20:20 +0200
Message-ID<tyLjY-Fc-17@gated-at.bofh.it>
In reply to#1627924
On Fri, Apr 21, 2017 at 10:22:52AM +0800, zhichang.yuan wrote:
> Hi, Dann,
> 
> 
> 
> On 04/21/2017 04:57 AM, dann frazier wrote:
> > On Thu, Mar 30, 2017 at 9:26 AM, zhichang.yuan
> > <yuanzhichang@hisilicon.com> wrote:
> >> On some platforms(such as Hip06/Hip07), the legacy ISA/LPC devices access I/O
> >> with some special host-local I/O ports known on x86. To access the I/O
> >> peripherals, an indirect-IO mechanism is introduced to mapped the host-local
> >> I/O to system logical/fake PIO similar the PCI MMIO on architectures where no
> >> separate I/O space exists. Just as PCI MMIO, the host I/O range should be
> >> registered before probing the downstream devices and set up the I/O mapping.
> >> But current ACPI bus probing doesn't support these indirect-IO hosts/devices.
> >>
> >> This patch introdueces a new ACPI handler for this device category. Through the
> >> handler attach callback, the indirect-IO hosts I/O registration is done and
> >> all peripherals' I/O resources are translated into logic/fake PIO before
> >> starting the enumeration.
> >>
> >> Signed-off-by: zhichang.yuan <yuanzhichang@hisilicon.com>
> >> Signed-off-by: Gabriele Paoloni <gabriele.paoloni@huawei.com>
> >> ---
> >>  drivers/acpi/Makefile          |   1 +
> >>  drivers/acpi/acpi_indirectio.c | 344 +++++++++++++++++++++++++++++++++++++++++
> >>  drivers/acpi/internal.h        |   5 +
> >>  drivers/acpi/scan.c            |   1 +
> >>  4 files changed, 351 insertions(+)
> >>  create mode 100644 drivers/acpi/acpi_indirectio.c
> >>
> >> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
> >> index a391bbc..10e5f2b 100644
> >> --- a/drivers/acpi/Makefile
> >> +++ b/drivers/acpi/Makefile
> >> @@ -57,6 +57,7 @@ acpi-$(CONFIG_ACPI_PROCFS_POWER) += cm_sbs.o
> >>  acpi-y                         += acpi_lpat.o
> >>  acpi-$(CONFIG_ACPI_GENERIC_GSI) += irq.o
> >>  acpi-$(CONFIG_ACPI_WATCHDOG)   += acpi_watchdog.o
> >> +acpi-$(CONFIG_INDIRECT_PIO)    += acpi_indirectio.o
> >>
> >>  # These are (potentially) separate modules
> >>
> >> diff --git a/drivers/acpi/acpi_indirectio.c b/drivers/acpi/acpi_indirectio.c
> >> new file mode 100644
> >> index 0000000..c8c80b5
> >> --- /dev/null
> >> +++ b/drivers/acpi/acpi_indirectio.c
> >> @@ -0,0 +1,344 @@
> 
> [snip]
> 
> >> +acpi_build_logiciores_template(struct acpi_device *adev,
> >> +                       struct acpi_buffer *buffer)
> >> +{
> >> +       acpi_handle handle = adev->handle;
> >> +       struct acpi_resource *resource;
> >> +       acpi_status status;
> >> +       int res_cnt = 0;
> >> +
> >> +       status = acpi_walk_resources(handle, METHOD_NAME__CRS,
> >> +                                    acpi_count_logiciores, &res_cnt);
> >> +       if (ACPI_FAILURE(status) || !res_cnt) {
> >> +               dev_err(&adev->dev, "can't evaluate _CRS: %d\n", status);
> >> +               return -EINVAL;
> >> +       }
> >> +
> >> +       buffer->length = sizeof(struct acpi_resource) * (res_cnt + 1) + 1;
> >> +       buffer->pointer = kzalloc(buffer->length - 1, GFP_KERNEL);
> > 
> > (Seth Forshee noticed this issue, just passing it on)
> > 
> > Should this just allocate the full buffer->length? That would keep the
> > length attribute accurate (possibly avoiding an off-by-1 error later).
> > It's not clear what the trailing byte is needed for, but other drivers
> > allocate it as well (drivers/acpi/pci_link.c and
> > drivers/platform/x86/sony-laptop.c).
> 
> Thanks for your suggestion!
> 
> I also curious why this one appended byte is needed as it seems the later
> acpi_set_current_resources() doesn't use this byte.
> And I tested without setting the buffer->length as the length of resource list
> directly, it seems ok.
> 
> But anyway, it looks more reasonable to allocate the memory with the
> buffer->length rather than buffer->length - 1;
> 
> I was made the V9 patch-set, and I can add your suggestion there. But I also
> awaiting for ARM64 ACPI maintainer's comment about this patch before really
> sending V9. I wonder whether there is better way to make our indirect-IO devices
> can be assigned the logic PIO before the enumeration...
> 
> Lorenzo, Hanjun, what do you think about this patch?

I will get to it shortly, sorry for the delay.

Thanks,
Lorenzo

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web