Re: [Patch V2 1/4] iommu/vt-d: replace *hdr with hdr[0] in struct dmar_drhd_unit

[Wei Yang]

On Mon, Mar 21, 2016 at 05:53:15PM +0100, Thomas Gleixner wrote:
>On Mon, 21 Mar 2016, Wei Yang wrote:
>> On Sun, Mar 20, 2016 at 04:42:29PM +0100, Thomas Gleixner wrote:
>> >On Sun, 20 Mar 2016, Wei Yang wrote:
>> >
>> >> hdr in struct dmar_drhd_unit is used to point the DMAR hardware unit copied
>> >> at the end of struct dmar_drhd_unit. One zero-sized array may be more
>> >> elegant for this purpose.
>> >
>> >You forget to tell why. 
>> > 
>> 
>> One possible benefit is to save some space.
>> 
>> Before commit 6b1972493a84 "iommu/vt-d: Implement DMAR unit hotplug framework",
>> dmaru->hdr just points to the memory region of DMA remapping hardware
>> definition. In this case, it would have no difference to where we put hdr.
>> 
>> After this commit, DMA remapping hardware definition is copied and
>> attach to the end of dmaru structure. By replacing a pointer with a zero-sized
>> array, that would save some space for this structure.
>
>Sure and exactly that explanation should be in the changelog. Not some
>handwaving "may be more elegant". We don't care about elegance, we care about
>correctness.
> 

Hey, Thomas

Nice to see your comment again~

You are right, I would add this in the changelog.

>> >> This patch replace *hdr with hdr[0] in struct dmar_drhd_unit.
>> >> 
>> >> Besides this, this patch includes other two changes:
>> >> 1. remove unnecessary type cast in dmar_table_detect()
>> >
>> >Again. Why is it not necessary?
>> >
>> 
>> The second parameter's type of function acpi_get_table_with_size() is "struct
>> acpi_table_header **", and type of dmar_tbl is "struct acpi_table_header *".
>> 
>> So without the type cast, the type of parameter and the function definition
>> matches.
>
>That's the information which a changelog wants to have, because otherwise a
>reviewer is forced to lookup the prototypes ....
>
>So a simple:
>
>   "Remove redundant type case to same type"
>
>would have told me what you are doing.
> 

Agree, thanks for your suggestion, this really helps reviewers.

>> >> 2. type cast from acpi_dmar_header to acpi_dmar_hardware_unit directly
>> >
>> >Don't even think about doing that. container_of() is there for a reason.
>> >
>> >Your change works today, because the embedded structure is the first one in
>> >the containing structure. If the containing structure gets reordered later,
>> >the whole thing will explode in hard to debug ways.
>> >
>> >Even if such a reordering is unlikely in that ACPI case, we just don't do
>> >that. It's bad and sloppy coding style. The generated code is the same.
>> >
>> 
>> Yes, I agree with you that make this change should be very careful, while I
>> think the original usage of container_of() is not necessary.
>
>It's not necessary, but it is correct. Removing it is just putting assumptions
>into the code, which is never a good idea.
>
>> Literally, it converts "struct acpi_dmar_header" to "struct
>> acpi_dmar_hardware_unit", because the first one is an element "header" of the
>> second one. While let's look at how the dmaru->hdr is initialized in
>> dmar_parse_one_drhd(), we copy the memory of "struct acpi_dmar_hardware_unit"
>> to a region where dmaru->hdr points to. So the code itself implies "struct
>> acpi_dmar_header" is the first element of "struct acpi_dmar_hardware_unit".
>u link
>Which is wrong to begin with. Assumption of first elements are crap.
>
>> Otherwise, we can't do this memcpy in dmar_parse_one_drhd().
> 
>> BTW, I am thinking changing the type of dmaru->hdr from "struct
>> acpi_dmar_header *" to "struct acpi_dmar_hardware_unit *". By doing so the
>> code would be more self explain, and it doesn't need to cast back and forth.
>
>Yes, that makes sense.
>

Ah, I am happy that you like this idea :-)

Let me format V3. Have a good night:)

>Thanks,
>
>	tglx

-- 
Wei Yang
Help you, Help me