Re: [PATCH rdma-next v2 09/11] RDMA/erdma: Add the erdma module

[Cheng Xu <chengyou@linux.alibaba.com>]

On 1/18/22 8:53 PM, Bernard Metzler wrote:

<...>

>> +static int erdma_res_cb_init(struct erdma_dev *dev)
>> +{
>> +	int i;
>> +
>> +	for (i = 0; i < ERDMA_RES_CNT; i++) {
>> +		dev->res_cb[i].next_alloc_idx = 1;
>> +		spin_lock_init(&dev->res_cb[i].lock);
>> +		dev->res_cb[i].bitmap = kcalloc(BITS_TO_LONGS(dev-
>>> res_cb[i].max_cap),
>> +						sizeof(unsigned long), GFP_KERNEL);
> 
> better stay with less than 80 chars per line
> throughout the patch series (I count currently 287 line wraps).
> 

The kernel now allows 100 chars per line, and the checkpath.pl also
checks using the new rule now. I will try to change this to 80 chars, 
but it actually makes some code not friendly for reading due to
indent.

<...>

>> +	switch (event) {
>> +	case NETDEV_UP:
>> +		dev->state = IB_PORT_ACTIVE;
>> +		erdma_port_event(dev, IB_EVENT_PORT_ACTIVE);
>> +		break;
>> +	case NETDEV_DOWN:
>> +		dev->state = IB_PORT_DOWN;
>> +		erdma_port_event(dev, IB_EVENT_PORT_ERR);
>> +		break;
>> +	case NETDEV_UNREGISTER:
>> +		ib_unregister_device_queued(ibdev);
>> +		break;
>> +	case NETDEV_REGISTER:
>> +	case NETDEV_CHANGEADDR:
> 
> No action needed here?
> 

Changing MAC address of ENIs in Alibaba Cloud makes no sense and is not
recommended, this will make the network unreachable and loss the 
connection. An ERDMA device is attached to an ENI, and no need to
process this event.

>> +	case NETDEV_CHANGEMTU:
>> +	case NETDEV_GOING_DOWN:
> 
> does erdma not have to take care about connected QPs
> if its associated link goes down? This event might be
> the right time to do cleanup, if needed (maybe also see siw
> driver)

The event of netdev down will be notified to the backend of virtio-net.
Virtio-net and ERDMA deivces are in the same MOC hardware (e,g, our DPU
chipset), and the event will be spread to ERDMA backend. After that,
ERDMA backend will process it, and notify to driver by AEQE or CQE with
error. No need to handle this in driver all together.

>> +	case NETDEV_CHANGE:
>> +	default:
>> +		break;
>> +	}
>> +
>> +	ib_device_put(ibdev);
>> +
>> +	return NOTIFY_DONE;
> 
> Better returning NOTIFY_OK here?
> 
> from <linux/notifier.h>:
>   #define NOTIFY_DONE     0x0000      /* Don't care */
>   #define NOTIFY_OK       0x0001      /* Suits me */
> 

I will check this.

Thanks,
Cheng Xu