Re: [PATCH 6/13] devres: implement managed iomap interface

Re: [PATCH 6/13] devres: implement managed iomap interface

[Sergei Shtylyov]

Tejun Heo wrote:

> +/**
> + * devm_ioremap - Managed ioremap()
> + * @dev: Generic device to remap IO address for
> + * @offset: BUS offset to map
> + * @size: Size of map
> + *
> + * Managed ioremap().  Map is automatically unmapped on driver detach.
> + */
> +void __iomem *devm_ioremap(struct device *dev, unsigned long offset,
> +			   unsigned long size)
> +{
> +	void __iomem **ptr, *addr;
> +
> +	ptr = devres_alloc(devm_ioremap_release, sizeof(*ptr), GFP_KERNEL);
> +	if (!ptr)
> +		return NULL;
> +
> +	addr = ioremap(offset, size);
> +	if (addr) {
> +		*ptr = addr;
> +		devres_add(dev, ptr);
> +	} else
> +		devres_free(ptr);
> +
> +	return addr;
> +}
> +EXPORT_SYMBOL(devm_ioremap);
> +
> +/**
> + * devm_ioremap_nocache - Managed ioremap_nocache()
> + * @dev: Generic device to remap IO address for
> + * @offset: BUS offset to map
> + * @size: Size of map
> + *
> + * Managed ioremap_nocache().  Map is automatically unmapped on driver
> + * detach.
> + */
> +void __iomem *devm_ioremap_nocache(struct device *dev, unsigned long offset,
> +				   unsigned long size)
> +{
> +	void __iomem **ptr, *addr;
> +
> +	ptr = devres_alloc(devm_ioremap_release, sizeof(*ptr), GFP_KERNEL);
> +	if (!ptr)
> +		return NULL;
> +
> +	addr = ioremap_nocache(offset, size);
> +	if (addr) {
> +		*ptr = addr;
> +		devres_add(dev, ptr);
> +	} else
> +		devres_free(ptr);
> +
> +	return addr;
> +}
> +EXPORT_SYMBOL(devm_ioremap_nocache);

    A very late comment but nevertheless... :-)
    Those functions are going to break on 32-bit platforms with extended 
physical address (well, that's starting with Pentiums which had 36-bit PAE :-) 
AND devices mapped beyond 4 GB (e.g. PowerPC 44x).  You should have used 
resource_size_t for the 'offset' parameter. As this most probably means that 
libata is broken on such platforms, I'm going to submit a patch...

WBR, Sergei

Re: [PATCH 6/13] devres: implement managed iomap interface

[Tejun Heo]

Sergei Shtylyov wrote:
>    A very late comment but nevertheless... :-)

Better late than never.

>    Those functions are going to break on 32-bit platforms with extended 
> physical address (well, that's starting with Pentiums which had 36-bit 
> PAE :-) AND devices mapped beyond 4 GB (e.g. PowerPC 44x).  You should 
> have used resource_size_t for the 'offset' parameter. As this most 
> probably means that libata is broken on such platforms, I'm going to 
> submit a patch...

Yeah, right please go ahead.  But I wonder whether any BIOS was actually 
crazy enough to map mmio region above 4G on 32bit machine.

-- 
tejun

Re: [PATCH 6/13] devres: implement managed iomap interface

[Sergei Shtylyov]

Tejun Heo wrote:

>>    A very late comment but nevertheless... :-)

> Better late than never.

    :-)

>>    Those functions are going to break on 32-bit platforms with 
>> extended physical address (well, that's starting with Pentiums which 
>> had 36-bit PAE :-) AND devices mapped beyond 4 GB (e.g. PowerPC 44x).  
>> You should have used resource_size_t for the 'offset' parameter. As 
>> this most probably means that libata is broken on such platforms, I'm 
>> going to submit a patch...

    It's broken with drivers using MMIO, I meant to say.

> Yeah, right please go ahead.  But I wonder whether any BIOS was actually 
> crazy enough to map mmio region above 4G on 32bit machine.

    This is a *hardware* mapping on some non-x86 platforms (like PPC 44x or 
MIPS Alchemy). The arch/ppc/ and arch/mips/ kernels have special hooks called 
from ioremap() which help create an illusion that the PCI memory space on such 
platforms (not only it) is mapped below 4 GB; arch/powerpc/ kernel doesn't do 
this anymore -- hence this newly encountered issue.

WBR, Sergei

Re: [PATCH 6/13] devres: implement managed iomap interface

[Tejun Heo]

Sergei Shtylyov wrote:
>> Yeah, right please go ahead.  But I wonder whether any BIOS was 
>> actually crazy enough to map mmio region above 4G on 32bit machine.
> 
>    This is a *hardware* mapping on some non-x86 platforms (like PPC 44x 
> or MIPS Alchemy). The arch/ppc/ and arch/mips/ kernels have special 
> hooks called from ioremap() which help create an illusion that the PCI 
> memory space on such platforms (not only it) is mapped below 4 GB; 
> arch/powerpc/ kernel doesn't do this anymore -- hence this newly 
> encountered issue.

Ah... I see.  Thanks for the clarification.

-- 
tejun

Re: [PATCH 6/13] devres: implement managed iomap interface

[Sergei Shtylyov]

Hello, I wrote:

>>>    Those functions are going to break on 32-bit platforms with 
>>> extended physical address (well, that's starting with Pentiums which 
>>> had 36-bit PAE :-) AND devices mapped beyond 4 GB (e.g. PowerPC 
>>> 44x).  You should have used resource_size_t for the 'offset' 
>>> parameter. As this most probably means that libata is broken on such 
>>> platforms, I'm going to submit a patch...

>    It's broken with drivers using MMIO, I meant to say.

    Oops, I meant PCI drivers here, at least for the time being. And it looks 
like that was a false alarm. :-]

>> Yeah, right please go ahead.  But I wonder whether any BIOS was 
>> actually crazy enough to map mmio region above 4G on 32bit machine.

>    This is a *hardware* mapping on some non-x86 platforms (like PPC 44x 
> or MIPS Alchemy). The arch/ppc/ and arch/mips/ kernels have special 
> hooks called from ioremap() which help create an illusion that the PCI 
> memory space on such platforms (not only it) is mapped below 4 GB; 
> arch/powerpc/ kernel doesn't do this anymore -- hence this newly 
> encountered issue.

    I thought that pcim_iomap() used devm_ioremap() or something -- which of 
course turned to be wrong. devm_ioremap() alone is yet safe since there are no 
users for it amongst PPC 44x platform device drivers...

MBR, Sergei

Re: [PATCH 6/13] devres: implement managed iomap interface

[Kumar Gala]

On Apr 10, 2008, at 11:56 AM, Sergei Shtylyov wrote:
> Hello, I wrote:
>
>>>>   Those functions are going to break on 32-bit platforms with  
>>>> extended physical address (well, that's starting with Pentiums  
>>>> which had 36-bit PAE :-) AND devices mapped beyond 4 GB (e.g.  
>>>> PowerPC 44x).  You should have used resource_size_t for the  
>>>> 'offset' parameter. As this most probably means that libata is  
>>>> broken on such platforms, I'm going to submit a patch...
>
>>   It's broken with drivers using MMIO, I meant to say.
>
>   Oops, I meant PCI drivers here, at least for the time being. And  
> it looks like that was a false alarm. :-]
>
>>> Yeah, right please go ahead.  But I wonder whether any BIOS was  
>>> actually crazy enough to map mmio region above 4G on 32bit machine.
>
>>   This is a *hardware* mapping on some non-x86 platforms (like PPC  
>> 44x or MIPS Alchemy). The arch/ppc/ and arch/mips/ kernels have  
>> special hooks called from ioremap() which help create an illusion  
>> that the PCI memory space on such platforms (not only it) is mapped  
>> below 4 GB; arch/powerpc/ kernel doesn't do this anymore -- hence  
>> this newly encountered issue.
>
>   I thought that pcim_iomap() used devm_ioremap() or something --  
> which of course turned to be wrong. devm_ioremap() alone is yet safe  
> since there are no users for it amongst PPC 44x platform device  
> drivers...

but there is no reason not to make it work properly.  For example I  
believe libata uses devm_* and the fsl SATA driver (non-PCI) will need  
to work in cases similar to the 44x.

- k

Re: [PATCH 6/13] devres: implement managed iomap interface

[Sergei Shtylyov]

Kumar Gala wrote:

>>>>>   Those functions are going to break on 32-bit platforms with  
>>>>> extended physical address (well, that's starting with Pentiums  
>>>>> which had 36-bit PAE :-) AND devices mapped beyond 4 GB (e.g.  
>>>>> PowerPC 44x).  You should have used resource_size_t for the  
>>>>> 'offset' parameter. As this most probably means that libata is  
>>>>> broken on such platforms, I'm going to submit a patch...

>>>   It's broken with drivers using MMIO, I meant to say.

>>   Oops, I meant PCI drivers here, at least for the time being. And  it 
>> looks like that was a false alarm. :-]

>>>> Yeah, right please go ahead.  But I wonder whether any BIOS was  
>>>> actually crazy enough to map mmio region above 4G on 32bit machine.

>>>   This is a *hardware* mapping on some non-x86 platforms (like PPC  
>>> 44x or MIPS Alchemy). The arch/ppc/ and arch/mips/ kernels have  
>>> special hooks called from ioremap() which help create an illusion  
>>> that the PCI memory space on such platforms (not only it) is mapped  
>>> below 4 GB; arch/powerpc/ kernel doesn't do this anymore -- hence  
>>> this newly encountered issue.

>>   I thought that pcim_iomap() used devm_ioremap() or something --  
>> which of course turned to be wrong. devm_ioremap() alone is yet safe  
>> since there are no users for it amongst PPC 44x platform device  
>> drivers...

> but there is no reason not to make it work properly.  For example I  
> believe libata uses devm_* and the fsl SATA driver (non-PCI) will need  
> to work in cases similar to the 44x.

    Well, as for sata_fsl, it calls of_iomap() which does The Right Thing.

> - k

WBR, Sergei

Re: [PATCH 6/13] devres: implement managed iomap interface

[Kumar Gala]

On Apr 10, 2008, at 1:24 PM, Sergei Shtylyov wrote:
> Kumar Gala wrote:
>
>>>>>>  Those functions are going to break on 32-bit platforms with   
>>>>>> extended physical address (well, that's starting with Pentiums   
>>>>>> which had 36-bit PAE :-) AND devices mapped beyond 4 GB (e.g.   
>>>>>> PowerPC 44x).  You should have used resource_size_t for the   
>>>>>> 'offset' parameter. As this most probably means that libata is   
>>>>>> broken on such platforms, I'm going to submit a patch...
>
>>>>  It's broken with drivers using MMIO, I meant to say.
>
>>>  Oops, I meant PCI drivers here, at least for the time being. And   
>>> it looks like that was a false alarm. :-]
>
>>>>> Yeah, right please go ahead.  But I wonder whether any BIOS was   
>>>>> actually crazy enough to map mmio region above 4G on 32bit  
>>>>> machine.
>
>>>>  This is a *hardware* mapping on some non-x86 platforms (like  
>>>> PPC  44x or MIPS Alchemy). The arch/ppc/ and arch/mips/ kernels  
>>>> have  special hooks called from ioremap() which help create an  
>>>> illusion  that the PCI memory space on such platforms (not only  
>>>> it) is mapped  below 4 GB; arch/powerpc/ kernel doesn't do this  
>>>> anymore -- hence  this newly encountered issue.
>
>>>  I thought that pcim_iomap() used devm_ioremap() or something --   
>>> which of course turned to be wrong. devm_ioremap() alone is yet  
>>> safe  since there are no users for it amongst PPC 44x platform  
>>> device  drivers...
>
>> but there is no reason not to make it work properly.  For example  
>> I  believe libata uses devm_* and the fsl SATA driver (non-PCI)  
>> will need  to work in cases similar to the 44x.
>
>   Well, as for sata_fsl, it calls of_iomap() which does The Right  
> Thing.

Fair, but I don't see why we should introduce new APIs that are  
already "broken".  We went through a lot of effort to clean up and  
introduce resource_t (and clearly still have some bugs) for the >32- 
bit physical address problem.

- k

Re: [PATCH 6/13] devres: implement managed iomap interface

[Tejun Heo]

Kumar Gala wrote:
>>> but there is no reason not to make it work properly.  For example I  
>>> believe libata uses devm_* and the fsl SATA driver (non-PCI) will 
>>> need  to work in cases similar to the 44x.
>>
>>   Well, as for sata_fsl, it calls of_iomap() which does The Right Thing.
> 
> Fair, but I don't see why we should introduce new APIs that are already 
> "broken".  We went through a lot of effort to clean up and introduce 
> resource_t (and clearly still have some bugs) for the >32-bit physical 
> address problem.

Yes, please go ahead.  In case it wasn't clear, I wasn't objecting to 
the fix at all.  I was just curious what could actually happen on x86.

-- 
tejun