Jan Beulich wrote:
> >>> Thomas Schlichter <[email protected]> 17.10.09 21:48 >>>
> >What do you think about these patches?
>
> Functionality-wise this looks fine to me; whether the core sysfs changes
> are acceptable I can't judge, though.
OK, who can? Or shall I simply ask get_maintainer.pl again?
> However, I would recommend folding the last two arguments of
> mtrr_add_unaligned(), i.e. use mtrr_usage != NULL as the increment
> argument passed to mtrr_add().
Good idea, I will do so this evening...
> And patches 5 and 6 would apparently not build right now, as they're
> failing to pass the new 5th argument to mtrr_add_unaligned().
*Grml* you are of course right. But I am not sure if these both patches
are a goot idea at all.
> Also, why do you add x86-specific code to drivers/pci-sysfs.c when the
> function called there (pci_mmap_page_range()) already is arch-specific?
> Moving your addition there would also allow covering the related
> (legacy?) procfs based functionality... pci_release() could also become
> arch-specific, with a fall-back definition to NULL.
You are right, I should do that...
> Additionally, I would suggest making those code portions depend on
> CONFIG_X86_MTRR rather than CONFIG_X86, so that you don't
> needlessly (try to) allocate the mtrr_usage vector.
Good idea, I will do so.
Thank you very much for your feedback, I'll hopefully come up with an
even better version tonight...
Kind regards,
Thomas
Jan Beulich wrote:
> Functionality-wise this looks fine to me; whether the core sysfs changes
> are acceptable I can't judge, though.
OK, I think I should have addressed your comments. Unfortunately I had to use
a little "hack" to make pci_mmap_page_range() work for sysfs and proc. I
placed a "private" pointer in the beginning of both per-file private
structures. So this pointer can be accessed independent from the caller. I
hope this is acceptable.
I dropped the ioremap() and set_memory_wc() patches, I could not implement
reference counting for them and it may interact too much with existing GPU
drivers.
Again, this series should not change the current behavior if either MTRR is
disabled or PAT is enabled. But it helps in the case that MTRR is enabled and
PAT is not available.
What should be done now to get this series on the right "track"?
Kind regards,
Thomas
* Thomas Schlichter <[email protected]> wrote:
> Jan Beulich wrote:
>
> > Functionality-wise this looks fine to me; whether the core sysfs
> > changes are acceptable I can't judge, though.
>
> OK, I think I should have addressed your comments. Unfortunately I had
> to use a little "hack" to make pci_mmap_page_range() work for sysfs
> and proc. I placed a "private" pointer in the beginning of both
> per-file private structures. So this pointer can be accessed
> independent from the caller. I hope this is acceptable.
>
> I dropped the ioremap() and set_memory_wc() patches, I could not
> implement reference counting for them and it may interact too much
> with existing GPU drivers.
>
> Again, this series should not change the current behavior if either
> MTRR is disabled or PAT is enabled. But it helps in the case that MTRR
> is enabled and PAT is not available.
>
> What should be done now to get this series on the right "track"?
Can we eliminate mtrr_add_unaligned() as Suresh suggested, and still
make it work on your testbox?
Once that's done i'll look at putting it into the x86 tree for testing.
The acks of Suresh/Venki/Jan would be nice to have.
Ingo