Re: [PATCH for-next 8/9] RDMA/hns: Kernel notify usr space to stop ring db

[Yangyang Li <liyangyang20@huawei.com>]

Hi, Leon & Jason
Thanks a lot for your reply.

在 2019/8/12 21:14, Jason Gunthorpe 写道:
> On Mon, Aug 12, 2019 at 08:52:20AM +0300, Leon Romanovsky wrote:
>> On Fri, Aug 09, 2019 at 05:41:05PM +0800, Lijun Ou wrote:
>>> From: Yangyang Li <liyangyang20@huawei.com>
>>>
>>> In the reset scenario, if the kernel receives the reset signal,
>>> it needs to notify the user space to stop ring doorbell.
>>
>> I doubt about it, it is racy like hell and relies on assumption that
>> userspace will honor such request to stop.
> 
> Sounds like this is the device unplug flow we already have support
> for, use the APIs to drop the VMA refering to the doorbell

Thanks for the suggestion, I have found this new unplug API,
and  I am trying to use it in the driver..

> 
>>> Signed-off-by: Yangyang Li <liyangyang20@huawei.com>
>>>  drivers/infiniband/hw/hns/hns_roce_device.h |  4 +++
>>>  drivers/infiniband/hw/hns/hns_roce_hw_v2.c  | 52 ++++++++++++++++++++++++++++-
>>>  drivers/infiniband/hw/hns/hns_roce_hw_v2.h  |  4 +++
>>>  drivers/infiniband/hw/hns/hns_roce_main.c   | 22 ++++++------
>>>  4 files changed, 70 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/drivers/infiniband/hw/hns/hns_roce_device.h b/drivers/infiniband/hw/hns/hns_roce_device.h
>>> index 32465f5..be65fce 100644
>>> +++ b/drivers/infiniband/hw/hns/hns_roce_device.h
>>> @@ -268,6 +268,8 @@ enum {
>>>
>>>  #define PAGE_ADDR_SHIFT				12
>>>
>>> +#define HNS_ROCE_IS_RESETTING			1
>>> +
>>>  struct hns_roce_uar {
>>>  	u64		pfn;
>>>  	unsigned long	index;
>>> @@ -1043,6 +1045,8 @@ struct hns_roce_dev {
>>>  	u32			odb_offset;
>>>  	dma_addr_t		tptr_dma_addr;	/* only for hw v1 */
>>>  	u32			tptr_size;	/* only for hw v1 */
>>> +	struct page		*reset_page; /* store reset state */
>>> +	void			*reset_kaddr; /* addr of reset page */
>>>  	const struct hns_roce_hw *hw;
>>>  	void			*priv;
>>>  	struct workqueue_struct *irq_workq;
>>> diff --git a/drivers/infiniband/hw/hns/hns_roce_hw_v2.c b/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
>>> index d33341e..138e5a8 100644
>>> +++ b/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
>>> @@ -1867,17 +1867,49 @@ static void hns_roce_free_link_table(struct hns_roce_dev *hr_dev,
>>>  			  link_tbl->table.map);
>>>  }
>>>
>>> +static int hns_roce_v2_get_reset_page(struct hns_roce_dev *hr_dev)
>>> +{
>>> +	hr_dev->reset_page = alloc_page(GFP_KERNEL | __GFP_ZERO);
>>> +	if (!hr_dev->reset_page)
>>> +		return -ENOMEM;
>>> +
>>> +	hr_dev->reset_kaddr = vmap(&hr_dev->reset_page, 1, VM_MAP, PAGE_KERNEL);
>>> +	if (!hr_dev->reset_kaddr)
>>> +		goto err_with_vmap;
> 
> Yes, this vmap is nonsense too, get_zeroed_page() is the right API
> 
> Jason
> 
> 
Thanks for the suggestion, put_page is not the correct usage,
I will use the appropriate API to fix it in the next patch.

Thanks