Hi,Liviu,
Thanks for your comments!
On 2016/11/10 0:50, liviu.dudau at arm.com wrote:
> On Wed, Nov 09, 2016 at 04:16:17PM +0000, Gabriele Paoloni wrote:
>> Hi Liviu
>>
>> Thanks for reviewing
>>
>
> [removed some irrelevant part of discussion, avoid crazy formatting]
>
>>>> +/**
>>>> + * addr_is_indirect_io - check whether the input taddr is for
>>> indirectIO.
>>>> + * @taddr: the io address to be checked.
>>>> + *
>>>> + * Returns 1 when taddr is in the range; otherwise return 0.
>>>> + */
>>>> +int addr_is_indirect_io(u64 taddr)
>>>> +{
>>>> + if (arm64_extio_ops->start > taddr || arm64_extio_ops->end <
>>> taddr)
>>>
>>> start >= taddr ?
>>
>> Nope... if (taddr < arm64_extio_ops->start || taddr > arm64_extio_ops->end)
>> then taddr is outside the range [start; end] and will return 0; otherwise
>> it will return 1...
>
> Oops, sorry, did not pay attention to the returned value. The check is
> correct as it is, no need to change then.
>
>>
>>>
>>>> + return 0;
>>>> +
>>>> + return 1;
>>>> +}
>>>>
>>>> BUILD_EXTIO(b, u8)
>>>>
>>>> diff --git a/drivers/of/address.c b/drivers/of/address.c
>>>> index 02b2903..cc2a05d 100644
>>>> --- a/drivers/of/address.c
>>>> +++ b/drivers/of/address.c
>>>> @@ -479,6 +479,50 @@ static int of_empty_ranges_quirk(struct
>>> device_node *np)
>>>> return false;
>>>> }
>>>>
>>>> +
>>>> +/*
>>>> + * of_isa_indirect_io - get the IO address from some isa reg
>>> property value.
>>>> + * For some isa/lpc devices, no ranges property in ancestor node.
>>>> + * The device addresses are described directly in their regs
>>> property.
>>>> + * This fixup function will be called to get the IO address of
>>> isa/lpc
>>>> + * devices when the normal of_translation failed.
>>>> + *
>>>> + * @parent: points to the parent dts node;
>>>> + * @bus: points to the of_bus which can be used to parse
>>> address;
>>>> + * @addr: the address from reg property;
>>>> + * @na: the address cell counter of @addr;
>>>> + * @presult: store the address paresed from @addr;
>>>> + *
>>>> + * return 1 when successfully get the I/O address;
>>>> + * 0 will return for some failures.
>>>
>>> Bah, you are returning a signed int, why 0 for failure? Return a
>>> negative value with
>>> error codes. Otherwise change the return value into a bool.
>>
>> Yes we'll move to bool
>>
>>>
>>>> + */
>>>> +static int of_get_isa_indirect_io(struct device_node *parent,
>>>> + struct of_bus *bus, __be32 *addr,
>>>> + int na, u64 *presult)
>>>> +{
>>>> + unsigned int flags;
>>>> + unsigned int rlen;
>>>> +
>>>> + /* whether support indirectIO */
>>>> + if (!indirect_io_enabled())
>>>> + return 0;
>>>> +
>>>> + if (!of_bus_isa_match(parent))
>>>> + return 0;
>>>> +
>>>> + flags = bus->get_flags(addr);
>>>> + if (!(flags & IORESOURCE_IO))
>>>> + return 0;
>>>> +
>>>> + /* there is ranges property, apply the normal translation
>>> directly. */
>>>
>>> s/there is ranges/if we have a 'ranges'/
>>
>> Thanks for spotting this
>>
>>>
>>>> + if (of_get_property(parent, "ranges", &rlen))
>>>> + return 0;
>>>> +
>>>> + *presult = of_read_number(addr + 1, na - 1);
>>>> + /* this fixup is only valid for specific I/O range. */
>>>> + return addr_is_indirect_io(*presult);
>>>> +}
>>>> +
>>>> static int of_translate_one(struct device_node *parent, struct
>>> of_bus *bus,
>>>> struct of_bus *pbus, __be32 *addr,
>>>> int na, int ns, int pna, const char *rprop)
>>>> @@ -595,6 +639,15 @@ static u64 __of_translate_address(struct
>>> device_node *dev,
>>>> result = of_read_number(addr, na);
>>>> break;
>>>> }
>>>> + /*
>>>> + * For indirectIO device which has no ranges property, get
>>>> + * the address from reg directly.
>>>> + */
>>>> + if (of_get_isa_indirect_io(dev, bus, addr, na, &result)) {
>>>> + pr_debug("isa indirectIO matched(%s)..addr =
>>> 0x%llx\n",
>>>> + of_node_full_name(dev), result);
>>>> + break;
>>>> + }
>>>>
>>>> /* Get new parent bus and counts */
>>>> pbus = of_match_bus(parent);
>>>> @@ -688,8 +741,9 @@ static int __of_address_to_resource(struct
>>> device_node *dev,
>>>> if (taddr == OF_BAD_ADDR)
>>>> return -EINVAL;
>>>> memset(r, 0, sizeof(struct resource));
>>>> - if (flags & IORESOURCE_IO) {
>>>> + if (flags & IORESOURCE_IO && taddr >= PCIBIOS_MIN_IO) {
>>>> unsigned long port;
>>>> +
>>>> port = pci_address_to_pio(taddr);
>>>> if (port == (unsigned long)-1)
>>>> return -EINVAL;
>>>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>>>> index ba34907..1a08511 100644
>>>> --- a/drivers/pci/pci.c
>>>> +++ b/drivers/pci/pci.c
>>>> @@ -3263,7 +3263,7 @@ int __weak pci_register_io_range(phys_addr_t
>>> addr, resource_size_t size)
>>>>
>>>> #ifdef PCI_IOBASE
>>>> struct io_range *range;
>>>> - resource_size_t allocated_size = 0;
>>>> + resource_size_t allocated_size = PCIBIOS_MIN_IO;
>>>>
>>>> /* check if the range hasn't been previously recorded */
>>>> spin_lock(&io_range_lock);
>>>> @@ -3312,7 +3312,7 @@ phys_addr_t pci_pio_to_address(unsigned long
>>> pio)
>>>>
>>>> #ifdef PCI_IOBASE
>>>> struct io_range *range;
>>>> - resource_size_t allocated_size = 0;
>>>> + resource_size_t allocated_size = PCIBIOS_MIN_IO;
>>>
>>> Have you checked that pci_pio_to_address still returns valid values
>>> after this? I know that
>>> you are trying to take into account PCIBIOS_MIN_IO limit when
>>> allocating reserving the IO ranges,
>>> but the values added in the io_range_list are still starting from zero,
>>> no from PCIBIOS_MIN_IO,
>>
>> I think you're wrong here as in pci_address_to_pio we have:
>> + resource_size_t offset = PCIBIOS_MIN_IO;
>>
>> This should be enough to guarantee that the PIOs start at
>> PCIBIOS_MIN_IO...right?
>
> I don't think you can guarantee that the pio value that gets passed into
> pci_pio_to_address() always comes from a previously returned value by
> pci_address_to_pio(). Maybe you can add a check in pci_pio_to_address()
>
> if (pio < PCIBIOS_MIN_IO)
> return address;
>
> to avoid adding more checks in the list_for_each_entry() loop.
>
I will register some ranges to the list and test it later.
But from my understanding, pci_pio_to_address() should can return the right
original physical address.
According to the algorithm, the output PIO ranges are consecutive， just like this:
input pio of pci_pio_to_address()
|
V
|----------------|--------------------------|------|-----------|
^
|
allocated_size is here
The change of this patch just make the start PIO from ZERO to PCIBIOS_MIN_IO.
in pci_pio_to_address(), for one input pio which fall into any PIO segment, the
return address will be:
address = range->start + pio - allocated_size;
Since allocated_size is the total range size of all IO ranges before the one
where pio belong, then (pio - allocated_size) is the offset to the range start,
So....
Thanks!
Zhichang
> Best regards,
> Liviu
>
>>
>>
>>> so the calculation of the address in this function could return
>>> negative values casted to pci_addr_t.
>>>
>>> Maybe you want to adjust the range->start value in
>>> pci_register_io_range() as well to have it
>>> offset by PCIBIOS_MIN_IO as well.
>>>
>>> Best regards,
>>> Liviu
>>>
>>>>
>>>> if (pio > IO_SPACE_LIMIT)
>>>> return address;
>>>> @@ -3335,7 +3335,7 @@ unsigned long __weak
>>> pci_address_to_pio(phys_addr_t address)
>>>> {
>>>> #ifdef PCI_IOBASE
>>>> struct io_range *res;
>>>> - resource_size_t offset = 0;
>>>> + resource_size_t offset = PCIBIOS_MIN_IO;
>>>> unsigned long addr = -1;
>>>>
>>>> spin_lock(&io_range_lock);
>>>> diff --git a/include/linux/of_address.h b/include/linux/of_address.h
>>>> index 3786473..deec469 100644
>>>> --- a/include/linux/of_address.h
>>>> +++ b/include/linux/of_address.h
>>>> @@ -24,6 +24,23 @@ struct of_pci_range {
>>>> #define for_each_of_pci_range(parser, range) \
>>>> for (; of_pci_range_parser_one(parser, range);)
>>>>
>>>> +
>>>> +#ifndef indirect_io_enabled
>>>> +#define indirect_io_enabled indirect_io_enabled
>>>> +static inline bool indirect_io_enabled(void)
>>>> +{
>>>> + return false;
>>>> +}
>>>> +#endif
>>>> +
>>>> +#ifndef addr_is_indirect_io
>>>> +#define addr_is_indirect_io addr_is_indirect_io
>>>> +static inline int addr_is_indirect_io(u64 taddr)
>>>> +{
>>>> + return 0;
>>>> +}
>>>> +#endif
>>>> +
>>>> /* Translate a DMA address from device space to CPU space */
>>>> extern u64 of_translate_dma_address(struct device_node *dev,
>>>> const __be32 *in_addr);
>>>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>>>> index 0e49f70..7f6bbb6 100644
>>>> --- a/include/linux/pci.h
>>>> +++ b/include/linux/pci.h
>>>> @@ -2130,4 +2130,12 @@ static inline bool pci_ari_enabled(struct
>>> pci_bus *bus)
>>>> /* provide the legacy pci_dma_* API */
>>>> #include <linux/pci-dma-compat.h>
>>>>
>>>> +/*
>>>> + * define this macro here to refrain from compilation error for some
>>>> + * platforms. Please keep this macro at the end of this header file.
>>>> + */
>>>> +#ifndef PCIBIOS_MIN_IO
>>>> +#define PCIBIOS_MIN_IO 0
>>>> +#endif
>>>> +
>>>> #endif /* LINUX_PCI_H */
>>>> --
>>>> 1.9.1
>>>>
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe linux-pci"
>>> in
>>>> the body of a message to majordomo at vger.kernel.org
>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>