Re: [RFC 01/11] acpi: arm: Public API for populating and query based on requesterid

[Julien Grall <julien.grall@linaro.org>]

On 19/01/18 06:05, Manish Jaggi wrote:
> 
> On 01/17/2018 12:01 AM, Julien Grall wrote:
>> Hi Manish,
>>
> Hi Julien,
> Thanks for reviewing the patch.
> 
>> I sent the previous e-mail too soon.
>>
>> On 02/01/18 09:27, manish.jaggi@linaro.org wrote:
>>> From: Manish Jaggi <manish.jaggi@linaro.org>
>>>
>>>   Public API to populate and query map between requester id and
>>>   streamId/DeviceID. IORT is parsed one time (outside this patch)
>>>   and two lists are created one for mapping between reuesterId and 
>>> streamid
>>>   and another between requesterID and deviceID.
>>>
>>>   These lists eliminate the need to reparse IORT for querying streamid
>>>   or deviceid using requesterid.
>>>
>>>   Signed-off-by: Manish Jaggi <manish.jaggi@linaro.org>
>>> ---
>>>   xen/drivers/acpi/Makefile     |   1 +
>>>   xen/drivers/acpi/arm/Makefile |   1 +
>>
>> We have a directory arch/arm/acpi/. So please move all your code there.
> The current files in arch/arm/acpi hold only boot time/low level code.
> IMHO creating drivers/acpi/arm makes more sense.
> Linux also has iort code in drivers/acpi/arm.

drivers/acpi mostly contain generic ACPI code. ridmap.c and iort.c is 
AFAICT Arm specific. So arch/arm/acpi is a better place.

[...]

>>
>>> + *
>>> + * Manish Jaggi <manish.jaggi@linaro.org>
>>> + * Copyright (c) 2018 Linaro.
>>> + *
>>> + * This program is free software; you can redistribute it and/or modify
>>> + * it under the terms of the GNU General Public License as published by
>>> + * the Free Software Foundation; either version 2 of the License, or
>>> + * (at your option) any later version.
>>
>> Xen is GPLv2 only and hence the copyright wrong. You want to use:
>>
>> This program is free software; you can redistribute it and/or modify it
>> under the terms and conditions of the GNU General Public License,
>> version 2, as published by the Free Software Foundation.
> 
> I picked this copyright from xen/arch/arm/traps.c.
> 
>   * xen/arch/arm/traps.c
>   *
>   * ARM Trap handlers
>   *
>   * Copyright (c) 2011 Citrix Systems.
>   *
>   * This program is free software; you can redistribute it and/or modify
>   * it under the terms of the GNU General Public License as published by
>   * the Free Software Foundation; either version 2 of the Licese, or
>   * (at your option) any later version.
> 
> So IIUYC, traps.c copyright is also wrong.
> How do we plan to fix all other files in xen code which use the same 
> copyright.

https://lists.xenproject.org/archives/html/xen-devel/2016-09/msg02899.html

>>> + *
>>> + * This program is distributed in the hope that it will be useful,
>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>> + * GNU General Public License for more details.
>>> + */
>>> +
>>> +#include <acpi/ridmap.h>
>>> +#include <xen/iommu.h>
>>> +#include <xen/kernel.h>
>>> +#include <xen/list.h>
>>> +#include <xen/pci.h>
>>> +
>>> +struct list_head rid_streamid_map_list;
>>> +struct list_head rid_deviceid_map_list;
>>
>> Please drop _list. This is pointless to know that when you can 
>> discover it.
> 
> I think it is not pointless.
> There is a point here. :)
>   _list is added to show that it is a list to make it more verbose.
> Without _list the name could mean a single mapping as well.

When I read, rid_streamid_map. I understand it is a map of rid/streamid. 
Not a single mapping. But ...

> 
> If you care to see
> xen/common/rangeset.c:27:    struct list_head range_list;
> 
> I hope you can appreciate the point.

... look at the name length here, 10 characters. Yours is 22 characters. 
This is 1/4 of a line. That's just stupid.

>>
>> Also, can you explain the rationale of using an unsorted list over 
>> another structure? 
> Since rid - streamId mapping also requires pcirc_node so it would 
> require two level of sorting.
> First sort based on pcirc_node and next on basis of rid.
> Does it makes sense to have all that complex code here ?
>   as this API will be used only once per pci device
>> Along that please give an idea how often and where the query API will 
>> be used.
> ok
> BTW, this is called from pci_for_each_dma_alias code flow.

The document the rationale.

>>
>>> +
>>> +void init_ridmaps(void)
>>
>> This likely need to be __init.
> ok.
>>
>>> +{
>>> +    INIT_LIST_HEAD(&rid_deviceid_map_list);
>>> +    INIT_LIST_HEAD(&rid_streamid_map_list);
>>> +}
>>
>> This function is not necessary. Declaring 
>> LIST_HEAD(rid_streamid_map_list) will do the trick.
> ok.
>>> +
>>> +int add_rid_streamid_map(struct acpi_iort_node *pcirc_node,
>>
>> Ditto.
> Ditto for? Sorry didnt catch your point here.

__init.

>>
>>> +                         struct acpi_iort_node *smmu_node,
>>> +                         u32 input_base, u32 output_base, u32 id_count)
>>
>> u32 & co should not be used in new code (unless imported from Linux). 
>> Please use uint32_t & co.
> I couldn't find this in xen coding style document.
> Could you please point to the section which says u32 should not be used.

It is not in the coding style but Xen is phasing out from u*. What's the 
problem?

> 
>>
>>> +{
>>> +    struct rid_streamid_map *rid_map;
>>
>> Newline here as it should be between after declarations.
>>
> ok
>>
>>> +    rid_map = xzalloc(struct rid_streamid_map);
>>> +
>>> +    if (!rid_map)
>>
>> This should be ( ... ).
>>
>>> +        return -ENOMEM;
>>
>> You either return -ENOMEM or 0 in this function. It sounds like to me 
>> that bool would be the best.
> I think ENOMEM should be used here. The error code is designed 
> specifically for this purpose.

Fair enough.

>>> +    return 0;
>>> +}
>>> +
>>> +void query_streamid(struct acpi_iort_node *pcirc_node, u16 rid, u32 
>>> *streamid,
>>
>> s/u*/uint_/
>>
>> But how come the rid is 16-bit here when Linux is using 32-bit?
> IIUC rid is 16bit only. Dont know why linux is using 32bit.
> 
> rid = bus - 8bits , devfn 8bits.
> 
>  From PCI Express specification
> The Requester ID is a 16-bit value that is unique for every PCI Express 
> Function within a Hierarchy..
> 
> If you think it is a 32bit value please let me know how to use upper 16 
> bits.

Well AFAICT, the IORT stores 32-bit. So it is probably best to stick 
with it.

> 
>>
>> Also, I am a bit puzzled how the caller is expected to use it. 
> I thought it would be self explanatory query streamid based on rid.
> But if it is not verbose enough for you, I will add this explicitly.

Not at all. More than this function is returning void. A query function 
is usually return a bool/int.

> 
>> From the name I would expect the function to return whether a 
>> translation was found. But it returns void.
>>
>> IHMO, this is a pretty bad idea and make more expectation on the value 
>> for the caller.
>>
>> Lastly, I would appreciate documentation on at least the function 
>> exported.
> ok.
>>
>>> +                    struct acpi_iort_node **smmu_node)
>>> +{
>>> +    struct rid_streamid_map *rmap;
>>> +
>>> +    list_for_each_entry(rmap, &rid_streamid_map_list, entry)
>>> +    {
>>> +        if (rmap->pcirc_node == pcirc_node)
>>
>> Coding style.
> Can we have a checkpatch.pl for xen.
> This would help in cases when code has a mix of files with linux coding 
> style and xen coding style.

This is been worked on.

[...]

>> I am not a big fan of exporting those 2 maps. But I will see how you 
>> use it before commenting.
> Lot of xen code using the same way. Is that all wrong?

Second sentence: "I will see how you use it before commenting".

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel