RE: [RFC v1 15/19] RDMA/irdma: Add miscellaneous utility definitions

["Saleem, Shiraz" <shiraz.saleem@intel.com>]

>Subject: Re: [RFC v1 15/19] RDMA/irdma: Add miscellaneous utility definitions
>
>On Fri, Feb 15, 2019 at 11:11:02AM -0600, Shiraz Saleem wrote:
>> From: Mustafa Ismail <mustafa.ismail@intel.com>
>>
>> Add miscellaneous utility functions and headers.
>>

[....]

>
>> +#define to_device(ptr)						\
>> +	(&((struct pci_dev *)((ptr)->hw->dev_context))->dev)
>
>?? Seems like this wants to be container_of??

Yes.

>
>> +/**
>> + * irdma_insert_wqe_hdr - write wqe header
>> + * @wqe: cqp wqe for header
>> + * @header: header for the cqp wqe
>> + */
>> +static inline void irdma_insert_wqe_hdr(__le64 *wqe, u64 hdr) {
>> +	wmb();   /* make sure WQE is populated before polarity is set */
>> +	set_64bit_val(wqe, 24, hdr);
>
>Generally don't like seeing wmbs in drivers.. Are you sure this isn't supposed to
>be smp_store_release(), or dma_wmb() perhaps?

Why is wmb() an issue in drivers?

>
>> +/**
>> + * irdma_inetaddr_event - system notifier for ipv4 addr events
>> + * @notfier: not used
>> + * @event: event for notifier
>> + * @ptr: if address
>> + */
>> +int irdma_inetaddr_event(struct notifier_block *notifier,
>> +			 unsigned long event,
>> +			 void *ptr)
>> +{
>> +	struct in_ifaddr *ifa = ptr;
>> +	struct net_device *event_netdev = ifa->ifa_dev->dev;
>> +	struct net_device *netdev;
>> +	struct net_device *upper_dev;
>> +	struct irdma_device *iwdev;
>> +	u32 local_ipaddr;
>> +
>> +	iwdev = irdma_find_netdev(event_netdev);
>
>This is all being changed too (and is probably wrongly locked here)

OK. We ll adapt it based on your new series. Thanks!

[...]

>
>> +/**
>> + * irdma_add_devusecount - add dev refcount
>> + * @iwdev: dev for refcount
>> + */
>> +void irdma_add_devusecount(struct irdma_device *iwdev) {
>> +	atomic64_inc(&iwdev->use_count);
>> +}
>> +
>> +/**
>> + * irdma_rem_devusecount - decrement refcount for dev
>> + * @iwdev: device
>> + */
>> +void irdma_rem_devusecount(struct irdma_device *iwdev) {
>> +	if (!atomic64_dec_and_test(&iwdev->use_count))
>> +		return;
>> +	wake_up(&iwdev->close_wq);
>> +}
>> +
>> +/**
>> + * irdma_add_pdusecount - add pd refcount
>> + * @iwpd: pd for refcount
>> + */
>> +void irdma_add_pdusecount(struct irdma_pd *iwpd) {
>> +	atomic_inc(&iwpd->usecount);
>> +}
>
>Why do we have these wrappers? Don't like wrappers liket his.
>
>Are you sure this should be an atomic, not a kref, refcount, etc?
>
>Very concerning to refcounting of HW object structures like this.. Most often when
>I see this in IB drivers it comes along with concurrency bugs in the destroy path.

The need for it is being re-visited and will likely go away.

>
>> +/**
>> + * irdma_allocate_dma_mem - Memory alloc helper fn
>> + * @hw:   pointer to the HW structure
>> + * @mem:  ptr to mem struct to fill out
>> + * @size: size of memory requested
>> + * @alignment: what to align the allocation to  */ enum
>> +irdma_status_code irdma_allocate_dma_mem(struct irdma_hw *hw,
>> +					      struct irdma_dma_mem *mem,
>> +					      u64 size,
>> +					      u32 alignment)
>> +{
>> +	struct pci_dev *pcidev = (struct pci_dev *)hw->dev_context;
>> +
>> +	if (!mem)
>> +		return IRDMA_ERR_PARAM;
>> +
>> +	mem->size = ALIGN(size, alignment);
>> +	mem->va = dma_alloc_coherent(&pcidev->dev, mem->size,
>> +				     (dma_addr_t *)&mem->pa, GFP_KERNEL);
>> +	if (!mem->va)
>> +		return IRDMA_ERR_NO_MEMORY;
>> +
>> +	return 0;
>> +}
>
>More wrappers? Why?

I agree on your previous comment on wrapper for usecnt tracking but this does
consolidate some common code and avoid duplication.

Shiraz