Re: [PATCH V5 2/3] ARM64 LPC: Add missing range exception for special ISA

[zhichang.yuan]

Hi,Liviu,

Thanks for your comments!


On 2016/11/10 0:50, liviu.dudau@xxxxxxx 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@xxxxxxxxxxxxxxx
>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>