On 14/04/15 11:33, Julien Grall wrote:
> On 14/04/15 10:28, Stefano Stabellini wrote:
>> On Tue, 14 Apr 2015, Jaggi, Manish wrote:
>>>> diff --git a/xen/include/asm-arm/pci.h b/xen/include/asm-arm/pci.h
>>>> index de13359..b8ec882 100644
>>>> --- a/xen/include/asm-arm/pci.h
>>>> +++ b/xen/include/asm-arm/pci.h
>>>> @@ -1,7 +1,8 @@
>>>> -#ifndef __X86_PCI_H__
>>>> -#define __X86_PCI_H__
>>>> +#ifndef __ARM_PCI_H__
>>>> +#define __ARM_PCI_H__
>>>>
>>>> struct arch_pci_dev {
>>>> + void *dev;
>>>
>>> void * is error-prone. Why can't you use the use the real structure?
>>>
>>> [manish]Will change it. I believe dev_archdata structure has also a void *
>>> (in asm-arm/device.h). So all void * are error prone in xen ?
>>>
>>
>> As you know void* works around the type system, so it prevents the
>> compiler from making many type safety checks. We should try to avoid
>> them if we can.
>
> In this case, the pointer add more management (allocation...).
>
> As for the device tree solution, the field should be a struct device.
>
>> I think that you are right, the void *iommu in dev_archdata should
>> actually be struct arm_smmu_xen_device *iommu.
>
> I agree that void* should be void in most of the case when we know the
> underlaying type. Although there is some place where the number of type
> of unknown because it could be used to store driver specific case.
>
> This is actually the case of this field. It contains private data for
> IOMMU driver. When a new driver will be implemented, it will likely use
> a different structure.
>
> Furthermore, the SMMU drivers is self contained in a file, using struct
> arm_smmu_xen_device* would require to export/define some part of the
> driver in an header.
A similar example would be the scheduler coder (see sched_data in
include/xen/sched-if.h). We don't want to expose the underlying
structure out of the file.
Regards,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
