Re: [PATCH i-g-t] lib: sync PCI ID macros with kernel

[Jani Nikula <jani.nikula@intel.com>]

On Wed, 13 Nov 2024, Zbigniew Kempczyński <zbigniew.kempczynski@intel.com> wrote:
> On Tue, Nov 12, 2024 at 03:02:10PM +0200, Jani Nikula wrote:
>
> <cut>
>
>> >
>> > I see no __KERNEL__ in current xe_pciids.h and i915_pciids.h so
>> > there's some change from my perspective. intel_vbt_defs.h also
>> > doesn't contain any kernel conditional inside. I think author
>> > of this change should provide additional documentation about this
>> > (not me) - commit message for me is not enough. Till this change
>> > I haven't noticed any __KERNEL__ usage in igt codebase and I think
>> > if it appears it should be documented what's for dead code is
>> > imported to the project.
>> 
>> See igt commit e966143f5c5f ("lib/intel_device_info: use dedicated macro
>> for struct pci_id_match init"). INTEL_VGA_DEVICE has been unused in igt
>> since then.
>> 
>> I wrapped the macro in #ifdef __KERNEL__ in kernel commit fc9cb46bdca8
>> ("drm/i915/pciids: use designated initializers in INTEL_VGA_DEVICE()")
>> to avoid it being used in igt again.
>
> Rationale of your change is clear to me - isolation of INTEL_VGA_DEVICE()
> to use in the kernel only. My problems are when I see such thing in the
> igt header
>
> #ifdef __KERNEL__
>
> ... some definitions ...
>
> #endif
>
> 1. is someone really is setting up __KERNEL__ in igt build system?
>    What's for it is here? (cognitive problem)
> 2. this conditional is not necessary here, let's remove it.
>    (dead code problem)
>
> I guess you will not accept change in which I would remove __KERNEL__
> conditional from the header (2).
>
>> 
>> I'm not going to document this further in igt.
>
> I'm asking for the documenting of this change not for me, but for
> other developers who might be confused by this conditional in the
> future. Especially we want to copy this header from the kernel.
> If you still think this is not necessary I have no other arguments
> to convince you to add some note to README.md.

I didn't say it's not necessary (nor did I say that it is). I said I
decline to write further documentation on this. If igt developers want
more documentation, they know what it needs to say, and I don't.


BR,
Jani.


-- 
Jani Nikula, Intel