Simon Horman <ho...@kernel.org> writes:
> On Mon, Apr 14, 2025 at 02:40:15PM -0500, Dave Marquardt wrote:
>> Replaced BUG_ON calls with WARN_ON calls with error handling, with
>> calls to a new ibmveth_reset routine, which resets the device. Removed
>> conflicting and unneeded forward declaration.
>
> To me the most important change here is adding the ibmveth_reset.
> So I would report that in the subject (rather than the WARN_ON) change.
> But perhaps that is just me.
Thanks, I'll consider that.
>>
>> Signed-off-by: Dave Marquardt <davem...@linux.ibm.com>
>> ---
>> drivers/net/ethernet/ibm/ibmveth.c | 116 ++++++++++++++++++++++++-----
>> drivers/net/ethernet/ibm/ibmveth.h | 65 ++++++++--------
>> 2 files changed, 130 insertions(+), 51 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/ibm/ibmveth.c
>> b/drivers/net/ethernet/ibm/ibmveth.c
>
> ...
>
>> @@ -370,20 +372,36 @@ static void ibmveth_free_buffer_pool(struct
>> ibmveth_adapter *adapter,
>> }
>> }
>>
>> -/* remove a buffer from a pool */
>> -static void ibmveth_remove_buffer_from_pool(struct ibmveth_adapter *adapter,
>> - u64 correlator, bool reuse)
>> +/**
>> + * ibmveth_remove_buffer_from_pool - remove a buffer from a pool
>> + * @adapter: adapter instance
>> + * @correlator: identifies pool and index
>> + * @reuse: whether to reuse buffer
>
> The above is the correct way to document function parameters in a Kernel doc.
>
>> + *
>> + * Return:
>> + * * %0 - success
>> + * * %-EINVAL - correlator maps to pool or index out of range
>> + * * %-EFAULT - pool and index map to null skb
>> + */
>> +static int ibmveth_remove_buffer_from_pool(struct ibmveth_adapter *adapter,
>> + u64 correlator, bool reuse)
>
> ...
>
>> +/**
>> + * ibmveth_rxq_harvest_buffer - Harvest buffer from pool
>> + *
>> + * @adapter - pointer to adapter
>> + * @reuse - whether to reuse buffer
>
> But this is not correct. IOW, tooling expects
> f.e. @adapter: ... rather than @adapter - ...
>
> Flagged by W=1 builds and ./scripts/kernel-doc -none
Thanks, I'll start using this in my work.
>> + *
>> + * Context: called from ibmveth_poll
>> + *
>> + * Return:
>> + * * %0 - success
>> + * * other - non-zero return from ibmveth_remove_buffer_from_pool
>> + */
>> +static int ibmveth_rxq_harvest_buffer(struct ibmveth_adapter *adapter,
>> + bool reuse)
>
> ...
>
>> diff --git a/drivers/net/ethernet/ibm/ibmveth.h
>> b/drivers/net/ethernet/ibm/ibmveth.h
>> index 8468e2c59d7a..b0a2460ec9f9 100644
>> --- a/drivers/net/ethernet/ibm/ibmveth.h
>> +++ b/drivers/net/ethernet/ibm/ibmveth.h
>> @@ -134,38 +134,39 @@ struct ibmveth_rx_q {
>> };
>>
>> struct ibmveth_adapter {
>> - struct vio_dev *vdev;
>> - struct net_device *netdev;
>> - struct napi_struct napi;
>> - unsigned int mcastFilterSize;
>> - void * buffer_list_addr;
>> - void * filter_list_addr;
>> - void *tx_ltb_ptr[IBMVETH_MAX_QUEUES];
>> - unsigned int tx_ltb_size;
>> - dma_addr_t tx_ltb_dma[IBMVETH_MAX_QUEUES];
>> - dma_addr_t buffer_list_dma;
>> - dma_addr_t filter_list_dma;
>> - struct ibmveth_buff_pool rx_buff_pool[IBMVETH_NUM_BUFF_POOLS];
>> - struct ibmveth_rx_q rx_queue;
>> - int rx_csum;
>> - int large_send;
>> - bool is_active_trunk;
>> -
>> - u64 fw_ipv6_csum_support;
>> - u64 fw_ipv4_csum_support;
>> - u64 fw_large_send_support;
>> - /* adapter specific stats */
>> - u64 replenish_task_cycles;
>> - u64 replenish_no_mem;
>> - u64 replenish_add_buff_failure;
>> - u64 replenish_add_buff_success;
>> - u64 rx_invalid_buffer;
>> - u64 rx_no_buffer;
>> - u64 tx_map_failed;
>> - u64 tx_send_failed;
>> - u64 tx_large_packets;
>> - u64 rx_large_packets;
>> - /* Ethtool settings */
>> + struct vio_dev *vdev;
>> + struct net_device *netdev;
>> + struct napi_struct napi;
>> + struct work_struct work;
>> + unsigned int mcastFilterSize;
>> + void *buffer_list_addr;
>> + void *filter_list_addr;
>> + void *tx_ltb_ptr[IBMVETH_MAX_QUEUES];
>> + unsigned int tx_ltb_size;
>> + dma_addr_t tx_ltb_dma[IBMVETH_MAX_QUEUES];
>> + dma_addr_t buffer_list_dma;
>> + dma_addr_t filter_list_dma;
>> + struct ibmveth_buff_pool rx_buff_pool[IBMVETH_NUM_BUFF_POOLS];
>> + struct ibmveth_rx_q rx_queue;
>> + int rx_csum;
>> + int large_send;
>> + bool is_active_trunk;
>> +
>> + u64 fw_ipv6_csum_support;
>> + u64 fw_ipv4_csum_support;
>> + u64 fw_large_send_support;
>> + /* adapter specific stats */
>> + u64 replenish_task_cycles;
>> + u64 replenish_no_mem;
>> + u64 replenish_add_buff_failure;
>> + u64 replenish_add_buff_success;
>> + u64 rx_invalid_buffer;
>> + u64 rx_no_buffer;
>> + u64 tx_map_failed;
>> + u64 tx_send_failed;
>> + u64 tx_large_packets;
>> + u64 rx_large_packets;
>> + /* Ethtool settings */
>> u8 duplex;
>> u32 speed;
>> };
>
> If you would like to update the indentation of this structure
> then please do so in a separate patch which precedes
> adding/removing/chainging fields of the structure.
>
> As it, it's very hard to see the non-formatting changes in this hunk.
I agree. Thanks for the suggestion.
-Dave
