On 7/15/19 2:47 PM, Matt Sickler wrote:
> It looks like Outlook is going to absolutely trash this email. Hopefully it
> comes through okay.
>
...
>>
>> Because this is a common pattern, and because the code here doesn't likely
>> need to set page dirty before the dma_unmap_sg call, I think the following
>> would be better (it's untested), instead of the above diff hunk:
>>
>> diff --git a/drivers/staging/kpc2000/kpc_dma/fileops.c
>> b/drivers/staging/kpc2000/kpc_dma/fileops.c
>> index 48ca88bc6b0b..d486f9866449 100644
>> --- a/drivers/staging/kpc2000/kpc_dma/fileops.c
>> +++ b/drivers/staging/kpc2000/kpc_dma/fileops.c
>> @@ -211,16 +211,13 @@ void transfer_complete_cb(struct aio_cb_data
>> *acd, size_t xfr_count, u32 flags)
>> BUG_ON(acd->ldev == NULL);
>> BUG_ON(acd->ldev->pldev == NULL);
>>
>> - for (i = 0 ; i < acd->page_count ; i++) {
>> - if (!PageReserved(acd->user_pages[i])) {
>> - set_page_dirty(acd->user_pages[i]);
>> - }
>> - }
>> -
>> dma_unmap_sg(&acd->ldev->pldev->dev, acd->sgt.sgl, acd->sgt.nents,
>> acd->ldev->dir);
>>
>> for (i = 0 ; i < acd->page_count ; i++) {
>> - put_page(acd->user_pages[i]);
>> + if (!PageReserved(acd->user_pages[i])) {
>> + put_user_pages_dirty(&acd->user_pages[i], 1);
>> + else
>> + put_user_page(acd->user_pages[i]);
>> }
>>
>> sg_free_table(&acd->sgt);
>
> I don't think I ever really knew the right way to do this.
>
> The changes Bharath suggested look okay to me. I'm not sure about the check
> for PageReserved(), though. At first glance it appears to be equivalent to
> what was there before, but maybe I should learn what that Reserved page flag
> really means.
> From [1], the only comment that seems applicable is
> * - MMIO/DMA pages. Some architectures don't allow to ioremap pages that are
> * not marked PG_reserved (as they might be in use by somebody else who does
> * not respect the caching strategy).
>
> These pages should be coming from anonymous (RAM, not file backed) memory in
> userspace. Sometimes it comes from hugepage backed memory, though I don't
> think that makes a difference. I should note that transfer_complete_cb
> handles both RAM to device and device to RAM DMAs, if that matters.
>
> [1]
> https://elixir.bootlin.com/linux/v5.2/source/include/linux/page-flags.h#L17
>
I agree: the PageReserved check looks unnecessary here, from my
outside-the-kpc_2000-team
perspective, anyway. Assuming that your analysis above is correct, you could
collapse that
whole think into just:
@@ -211,17 +209,8 @@ void transfer_complete_cb(struct aio_cb_data *acd, size_t
xfr_count, u32 flags)
BUG_ON(acd->ldev == NULL);
BUG_ON(acd->ldev->pldev == NULL);
- for (i = 0 ; i < acd->page_count ; i++) {
- if (!PageReserved(acd->user_pages[i])) {
- set_page_dirty(acd->user_pages[i]);
- }
- }
-
dma_unmap_sg(&acd->ldev->pldev->dev, acd->sgt.sgl, acd->sgt.nents,
acd->ldev->dir);
-
- for (i = 0 ; i < acd->page_count ; i++) {
- put_page(acd->user_pages[i]);
- }
+ put_user_pages_dirty(&acd->user_pages[i], acd->page_count);
sg_free_table(&acd->sgt);
(Also, Matt, I failed to Cc: you on a semi-related cleanup that I just sent out
for this
driver, as long as I have your attention:
https://lore.kernel.org/r/20190715212123.432-1-jhubb...@nvidia.com
)
thanks,
--
John Hubbard
NVIDIA
_______________________________________________
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
