Re: [nicholas.johnson-opensource@outlook.com.au: [PATCH v6 4/4] PCI: Add pci=hpmemprefsize parameter to set MMIO_PREF size independently]

[Logan Gunthorpe <logang@deltatee.com>]

On 2019-06-21 12:01 a.m., Nicholas Johnson wrote:
> Thanks for advice. I do believe you are correct. I guess it is my 
> inexperience - I have trouble telling what is normal and what to expect 
> from the process. But I am not feeling like this is any closer to 
> submission than on day one - no sense of progress. It is not reassuring 
> and I do wonder if I will ever manage to get it right.
> 
> None of this is technically Thunderbolt-specific, but that is the use 
> case, and if a single one of these patches does not make it in then the 
> use case of Thunderbolt without firmware support will remain broken. The 
> only benefit of a partial acception is patch #1 which fixes a bug report 
> by Mika Westerberg on its own, so I guess that would be a win. If it has 
> to wait another cycle then I will live with it, but this is the cause of 
> my anxiety.
> 
> I can look at this patch again, but my concerns are the following:
> 
> - If we avoid depreciation, then that implies this patch is ideal - we 
> are not breaking anything for existing users. We can set MMIO on its own 
> with pci=hpmemsize and by setting pci=hpmemprefsize to the default size of 
> 2M (so it remains unchanged).

I disagree. The semantics are weird and the changes to the documentation
show that. When you have to describe things based on what other
parameters are doing, it's weird.

> - If we have two sets of parameters, then we have to support having two 
> sets for eternity, and the loss of minimalism and the potential for 
> confusion. The description in kernel-parameters.txt will become 
> confusing with a lot of "if this" and "if that".

Yes, but they're simple and clear parameters that are easy to support.
There would be no "ifs" at all. One parameter sets one value, another a
parameter sets another, and the original parameter sets both.

> - How do we deal with order if both are specified? It will become 
> unclean.

If a user mixes the "both" with the "single" parameters, the winner goes
to the last that's specified. This is common and predictable. It's not
dissimilar to when a user specifies the same parameter twice.

> - If we make two new ones, then what should they be called? I propose 
> hp_mmio_size and hp_mmio_pref_size from my original submission. Then 
> should hpiosize be aliased to hp_io_size to keep the consistency?

Good question. I'd probably go something along the lines of
'hpmemsize_pref' and 'hpmemsize_nonpref'. Though, it might be good to
get others opinions on this.

> - I am quite surprised that when 64-bit window support was added to the 
> kernel, that this was not done then, although I am not good enough at 
> navigating the mailing list to see what actually happened.

Generally, things won't get done until people have a use case for them
and motivation to make the change. I don't think there's a lot of users
of hpmemsize to begin with.

> Should I perhaps forward my original submission for this patch again and 
> you can see if there is anything to salvage from that that?

Not if you haven't changed anything.

Logan