Re: [PATCH net] hinic: fix wrong return value of mac-set cmd
On 2020/9/24 8:43, David Miller wrote: > From: Luo bin > Date: Tue, 22 Sep 2020 19:26:43 +0800 > >> It should also be regarded as an error when hw return status=4 for PF's >> setting mac cmd. Only if PF return status=4 to VF should this cmd be >> taken special treatment. >> >> Signed-off-by: Luo bin > > Bug fixes require a proper Fixes: tag. > > Please resubmit with the corrected, thank you. > . > Will fix. Thanks!
Re: [PATCH] hinic: fix potential resource leak
On 2020/9/17 11:03, Wei Li wrote: > + err = irq_set_affinity_hint(rq->irq, >affinity_mask); > + if (err) > + goto err_irq; > + > + return 0; > + > +err_irq: > + rx_del_napi(rxq); > + return err; If irq_set_affinity_hint fails, irq should be freed as well.
Re: [PATCH net] hinic: fix rewaking txq after netif_tx_disable
On 2020/9/8 5:28, Jakub Kicinski wrote: > On Mon, 7 Sep 2020 22:15:16 +0800 Luo bin wrote: >> When calling hinic_close in hinic_set_channels, all queues are >> stopped after netif_tx_disable, but some queue may be rewaken in >> free_tx_poll by mistake while drv is handling tx irq. If one queue >> is rewaken core may call hinic_xmit_frame to send pkt after >> netif_tx_disable within a short time which may results in accessing >> memory that has been already freed in hinic_close. So we judge >> whether the netdev is in down state before waking txq in free_tx_poll >> to fix this bug. > > The right fix is to call napi_disable() _before_ you call > netif_tx_disable(), not after, like hinic_close() does. > . > Will fix. Thanks for your review.
Re: [PATCH net 3/3] hinic: fix bug of send pkts while setting channels
On 2020/9/2 18:16, Eric Dumazet wrote:
>
>
> On 9/2/20 2:41 AM, Luo bin wrote:
>> When calling hinic_close in hinic_set_channels, netif_carrier_off
>> and netif_tx_disable are excuted, and TX host resources are freed
>> after that. Core may call hinic_xmit_frame to send pkt after
>> netif_tx_disable within a short time, so we should judge whether
>> carrier is on before sending pkt otherwise the resources that
>> have already been freed in hinic_close may be accessed.
>>
>> Fixes: 2eed5a8b614b ("hinic: add set_channels ethtool_ops support")
>> Signed-off-by: Luo bin
>> ---
>> drivers/net/ethernet/huawei/hinic/hinic_tx.c | 5 +
>> 1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/huawei/hinic/hinic_tx.c
>> b/drivers/net/ethernet/huawei/hinic/hinic_tx.c
>> index a97498ee6914..a0662552a39c 100644
>> --- a/drivers/net/ethernet/huawei/hinic/hinic_tx.c
>> +++ b/drivers/net/ethernet/huawei/hinic/hinic_tx.c
>> @@ -531,6 +531,11 @@ netdev_tx_t hinic_xmit_frame(struct sk_buff *skb,
>> struct net_device *netdev)
>> struct hinic_txq *txq;
>> struct hinic_qp *qp;
>>
>> +if (unlikely(!netif_carrier_ok(netdev))) {
>> +dev_kfree_skb_any(skb);
>> +return NETDEV_TX_OK;
>> +}
>> +
>> txq = _dev->txqs[q_id];
>> qp = container_of(txq->sq, struct hinic_qp, sq);
>>
>>
>
> Adding this kind of tests in fast path seems a big hammer to me.
>
> See https://marc.info/?l=linux-netdev=159903844423389=2 for a similar
> problem.
>
> Normally, after hinic_close() operation, no packet should be sent by core
> networking stack.
>
> Trying to work around some core networking issue in each driver is a dead end.
Thanks for your review. I agree with what you said. Theoretically, core can't
call ndo_start_xmit
to send packet after netif_tx_disable called by hinic_close because
__QUEUE_STATE_DRV_XOFF bit is set
and this bit is protected by __netif_tx_lock but it does call hinic_xmit_frame
after netif_tx_disable
in my debug message. I'll try to figure out why and fix it. It seems like that
the patch from
https://marc.info/?l=linux-netdev=159903844423389=2 can't fix this problem.
>
>
>
>
>
>
> .
>
Re: [PATCH net 3/3] hinic: fix bug of send pkts while setting channels
On 2020/9/3 3:52, David Miller wrote:
> From: Luo bin
> Date: Wed, 2 Sep 2020 17:41:45 +0800
>
>> @@ -531,6 +531,11 @@ netdev_tx_t hinic_xmit_frame(struct sk_buff *skb,
>> struct net_device *netdev)
>> struct hinic_txq *txq;
>> struct hinic_qp *qp;
>>
>> +if (unlikely(!netif_carrier_ok(netdev))) {
>> +dev_kfree_skb_any(skb);
>> +return NETDEV_TX_OK;
>> +}
>
> As Eric said, these kinds of tests should not be placed in the fast path
> of the driver.
>
> If you invoke close and the core networking still sends packets to the
> driver, that's a bug that needs to be fixed in the core networking.
> .
>
Okay, I'm trying to figure out why the core networking can still call
ndo_start_xmit
after netif_tx_disable and solve the problem fundamentally. And I'll undo this
patch
temporarily.
Re: [PATCH net-next v1 3/3] hinic: add support to query function table
On 2020/8/29 1:19, Jakub Kicinski wrote:
> On Fri, 28 Aug 2020 11:16:22 +0800 luobin (L) wrote:
>> On 2020/8/28 3:44, Jakub Kicinski wrote:
>>> On Thu, 27 Aug 2020 19:13:21 +0800 Luo bin wrote:
>>>> + switch (idx) {
>>>> + case VALID:
>>>> + return funcfg_table_elem->dw0.bs.valid;
>>>> + case RX_MODE:
>>>> + return funcfg_table_elem->dw0.bs.nic_rx_mode;
>>>> + case MTU:
>>>> + return funcfg_table_elem->dw1.bs.mtu;
>>>> + case VLAN_MODE:
>>>> + return funcfg_table_elem->dw1.bs.vlan_mode;
>>>> + case VLAN_ID:
>>>> + return funcfg_table_elem->dw1.bs.vlan_id;
>>>> + case RQ_DEPTH:
>>>> + return funcfg_table_elem->dw13.bs.cfg_rq_depth;
>>>> + case QUEUE_NUM:
>>>> + return funcfg_table_elem->dw13.bs.cfg_q_num;
>>>
>>> The first two patches look fairly unobjectionable to me, but here the
>>> information does not seem that driver-specific. What's vlan_mode, and
>>> vlan_id in the context of PF? Why expose mtu, is it different than
>>> netdev mtu? What's valid? rq_depth?
>>> .
>>>
>> The vlan_mode and vlan_id in function table are provided for VF in QinQ
>> scenario
>> and they are useless for PF. Querying VF's function table is unsupported
>> now, so
>> there is no need to expose vlan_id and vlan mode and I'll remove them in my
>> next
>> patchset. The function table is saved in hw and we expose the mtu to ensure
>> the
>> mtu saved in hw is same with netdev mtu. The valid filed indicates whether
>> this
>> function is enabled or not and the hw can judge whether the RQ buffer in
>> host is
>> sufficient by comparing the values of rq depth, pi and ci.
>
> Queue depth is definitely something we can add to the ethtool API.
> You already expose raw producer and consumer indexes so the calculation
> can be done, anyway.
> .
>
Okay, I'll remove the queue depth as well. Thanks for your review.
Re: [PATCH net-next v1 3/3] hinic: add support to query function table
On 2020/8/28 3:44, Jakub Kicinski wrote:
> On Thu, 27 Aug 2020 19:13:21 +0800 Luo bin wrote:
>> +switch (idx) {
>> +case VALID:
>> +return funcfg_table_elem->dw0.bs.valid;
>> +case RX_MODE:
>> +return funcfg_table_elem->dw0.bs.nic_rx_mode;
>> +case MTU:
>> +return funcfg_table_elem->dw1.bs.mtu;
>> +case VLAN_MODE:
>> +return funcfg_table_elem->dw1.bs.vlan_mode;
>> +case VLAN_ID:
>> +return funcfg_table_elem->dw1.bs.vlan_id;
>> +case RQ_DEPTH:
>> +return funcfg_table_elem->dw13.bs.cfg_rq_depth;
>> +case QUEUE_NUM:
>> +return funcfg_table_elem->dw13.bs.cfg_q_num;
>
> The first two patches look fairly unobjectionable to me, but here the
> information does not seem that driver-specific. What's vlan_mode, and
> vlan_id in the context of PF? Why expose mtu, is it different than
> netdev mtu? What's valid? rq_depth?
> .
>
The vlan_mode and vlan_id in function table are provided for VF in QinQ scenario
and they are useless for PF. Querying VF's function table is unsupported now, so
there is no need to expose vlan_id and vlan mode and I'll remove them in my next
patchset. The function table is saved in hw and we expose the mtu to ensure the
mtu saved in hw is same with netdev mtu. The valid filed indicates whether this
function is enabled or not and the hw can judge whether the RQ buffer in host is
sufficient by comparing the values of rq depth, pi and ci.
Re: [PATCH net-next] hinic: add debugfs support
On 2020/8/21 0:02, Jakub Kicinski wrote:
> On Thu, 20 Aug 2020 20:14:32 +0800 Luo bin wrote:
>> +static int hinic_dbg_help(struct hinic_dev *nic_dev, const char *cmd_buf)
>> +{
>> +netif_info(nic_dev, drv, nic_dev->netdev, "Available commands:\n");
>> +netif_info(nic_dev, drv, nic_dev->netdev, "sq info \n");
>> +netif_info(nic_dev, drv, nic_dev->netdev, "sq wqe info > id>\n");
>> +netif_info(nic_dev, drv, nic_dev->netdev, "rq info \n");
>> +netif_info(nic_dev, drv, nic_dev->netdev, "rq wqe info > id>\n");
>> +netif_info(nic_dev, drv, nic_dev->netdev, "sq ci table \n");
>> +netif_info(nic_dev, drv, nic_dev->netdev, "rq cqe info > id>\n");
>> +netif_info(nic_dev, drv, nic_dev->netdev, "mac table\n");
>> +netif_info(nic_dev, drv, nic_dev->netdev, "global table\n");
>> +netif_info(nic_dev, drv, nic_dev->netdev, "func table\n");
>> +netif_info(nic_dev, drv, nic_dev->netdev, "port table\n");
>> +return 0;
>> +}
>> +
>> +static const struct hinic_dbg_cmd_info g_hinic_dbg_cmd[] = {
>> +{"help", hinic_dbg_help},
>> +{"sq info", hinic_dbg_get_sq_info},
>> +{"sq wqe info", hinic_dbg_get_sq_wqe_info},
>> +{"rq info", hinic_dbg_get_rq_info},
>> +{"rq wqe info", hinic_dbg_get_rq_wqe_info},
>> +{"sq ci table", hinic_dbg_get_ci_table},
>> +{"rq cqe info", hinic_dbg_get_rq_cqe_info},
>> +{"mac table", hinic_dbg_get_mac_table},
>> +{"global table", hinic_dbg_get_global_table},
>> +{"func table", hinic_dbg_get_function_table},
>> +{"port table", hinic_dbg_get_port_table},
>> +};
>
> Please don't create command interfaces like this.
>
> Instead create a read only file for objects you want to expose.
>
> Split addition of each object into a separate patch and provide example
> output in the commit message.
> .
>
Will fix. Thanks for your review.
Re: [PATCH net-next v1] hinic: fix strncpy output truncated compile warnings
On 2020/8/8 20:50, David Laight wrote: > From: luobin (L) >> Sent: 08 August 2020 04:37 >> >> On 2020/8/7 17:32, David Laight wrote: >>> From: Luo bin >>>> Sent: 07 August 2020 03:09 >>>> >>>> fix the compile warnings of 'strncpy' output truncated before >>>> terminating nul copying N bytes from a string of the same length >>>> >>>> Signed-off-by: Luo bin >>>> Reported-by: kernel test robot >>>> --- >>>> V0~V1: >>>> - use the strlen()+1 pattern consistently >>>> >>>> drivers/net/ethernet/huawei/hinic/hinic_devlink.c | 8 >>>> 1 file changed, 4 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/drivers/net/ethernet/huawei/hinic/hinic_devlink.c >>>> b/drivers/net/ethernet/huawei/hinic/hinic_devlink.c >>>> index c6adc776f3c8..1ec88ebf81d6 100644 >>>> --- a/drivers/net/ethernet/huawei/hinic/hinic_devlink.c >>>> +++ b/drivers/net/ethernet/huawei/hinic/hinic_devlink.c >>>> @@ -342,9 +342,9 @@ static int chip_fault_show(struct devlink_fmsg *fmsg, >>>> >>>>level = event->event.chip.err_level; >>>>if (level < FAULT_LEVEL_MAX) >>>> - strncpy(level_str, fault_level[level], >>>> strlen(fault_level[level])); >>>> + strncpy(level_str, fault_level[level], >>>> strlen(fault_level[level]) + 1); >>> >>> Have you even considered what that code is actually doing? >>> >>> David >> >> I'm sorry that I haven't got what you mean and I haven't found any defects >> in that code. Can you >> explain more to me? > > If you can't see it you probably shouldn't be submitting patches > > Consider what happens when the string is long. > > David > > - > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 > 1PT, UK > Registration No: 1397386 (Wales) > Thanks for your explanation and review. The fault_level[level] is a fixed and NUL-terminated character string in that code and its length is smaller than the dest buffer size so I think using strlen(fault_level[level]) + 1 will not overflow the destination buffer. But using strncpy() on NUL-terminated strings is dangerous indeed and there is totally no need to use it in that code as Kees points out.
Re: [PATCH net-next v1] hinic: fix strncpy output truncated compile warnings
On 2020/8/8 14:44, Kees Cook wrote:
> On Fri, Aug 07, 2020 at 08:42:43PM -0700, David Miller wrote:
>> From: "luobin (L)"
>> Date: Sat, 8 Aug 2020 11:36:42 +0800
>>
>>> On 2020/8/7 17:32, David Laight wrote:
>>>>> diff --git a/drivers/net/ethernet/huawei/hinic/hinic_devlink.c
>>>>> b/drivers/net/ethernet/huawei/hinic/hinic_devlink.c
>>>>> index c6adc776f3c8..1ec88ebf81d6 100644
>>>>> --- a/drivers/net/ethernet/huawei/hinic/hinic_devlink.c
>>>>> +++ b/drivers/net/ethernet/huawei/hinic/hinic_devlink.c
>>>>> @@ -342,9 +342,9 @@ static int chip_fault_show(struct devlink_fmsg *fmsg,
>>>>>
>>>>> level = event->event.chip.err_level;
>>>>> if (level < FAULT_LEVEL_MAX)
>>>>> - strncpy(level_str, fault_level[level],
>>>>> strlen(fault_level[level]));
>>>>> + strncpy(level_str, fault_level[level],
>>>>> strlen(fault_level[level]) + 1);
>>>>
>>>> Have you even considered what that code is actually doing?
>> ...
>>> I'm sorry that I haven't got what you mean and I haven't found any defects
>>> in that code. Can you explain more to me?
>>
>> David is trying to express the same thing I was trying to explain to
>> you, you should use sizeof(level_str) as the third argument because
>> the code is trying to make sure that the destination buffer is not
>> overrun.
>>
>> If you use the strlen() of the source buffer, the strncpy() can still
>> overflow the destination buffer.
>>
>> Now do you understand?
>
> Agh, please never use strncpy() on NUL-terminated strings[1]. (You can
> see this ultimately gets passed down into devlink_fmsg_string_put()
> which expects NUL-terminated strings and does not require trailing
> NUL-padding (which if it did, should still never use strncpy(), but
> rather strscpy_pad()).
>
> But, as David Laight hints, none of this is needed. The entire buffer
> can be avoided (just point into the existing array of strings -- which
> should also be const). Add I see that one of the array sizes is wrong.
> Both use FAULT_TYPE_MAX, but one should be FAULT_LEVEL_MAX. And since
> "Unknown" can just be added to the array, do that and clamp the value
> since it's only used for finding the strings in the array.
>
> I would suggest this (totally untested):
>
> diff --git a/drivers/net/ethernet/huawei/hinic/hinic_devlink.c
> b/drivers/net/ethernet/huawei/hinic/hinic_devlink.c
> index c6adc776f3c8..20bfb05896e5 100644
> --- a/drivers/net/ethernet/huawei/hinic/hinic_devlink.c
> +++ b/drivers/net/ethernet/huawei/hinic/hinic_devlink.c
> @@ -334,18 +334,12 @@ void hinic_devlink_unregister(struct hinic_devlink_priv
> *priv)
> static int chip_fault_show(struct devlink_fmsg *fmsg,
> struct hinic_fault_event *event)
> {
> - char fault_level[FAULT_TYPE_MAX][FAULT_SHOW_STR_LEN + 1] = {
> - "fatal", "reset", "flr", "general", "suggestion"};
> - char level_str[FAULT_SHOW_STR_LEN + 1] = {0};
> - u8 level;
> + const char * const level_str[FAULT_LEVEL_MAX + 1] = {
> + "fatal", "reset", "flr", "general", "suggestion",
> + [FAULT_LEVEL_MAX] = "Unknown"};
> + u8 fault_level;
> int err;
>
> - level = event->event.chip.err_level;
> - if (level < FAULT_LEVEL_MAX)
> - strncpy(level_str, fault_level[level],
> strlen(fault_level[level]));
> - else
> - strncpy(level_str, "Unknown", strlen("Unknown"));
> -
> if (level == FAULT_LEVEL_SERIOUS_FLR) {
> err = devlink_fmsg_u32_pair_put(fmsg, "Function level err
> func_id",
> (u32)event->event.chip.func_id);
> @@ -361,7 +355,8 @@ static int chip_fault_show(struct devlink_fmsg *fmsg,
> if (err)
> return err;
>
> - err = devlink_fmsg_string_pair_put(fmsg, "err_level", level_str);
> + fault_level = clamp(event->event.chip.err_level, FAULT_LEVEL_MAX);
> + err = devlink_fmsg_string_pair_put(fmsg, "err_level",
> fault_str[fault_level]);
> if (err)
> return err;
>
> @@ -381,18 +376,15 @@ static int chip_fault_show(struct devlink_fmsg *fmsg,
> static int fault_report_show(struct devlink_fmsg *fmsg,
>struct hinic_fault_event *event)
> {
> - char fault_type[FAULT_TYPE_MAX][FAU
Re: [PATCH net-next v1] hinic: fix strncpy output truncated compile warnings
On 2020/8/8 11:42, David Miller wrote: > From: "luobin (L)" > Date: Sat, 8 Aug 2020 11:36:42 +0800 > >> On 2020/8/7 17:32, David Laight wrote: >>>> diff --git a/drivers/net/ethernet/huawei/hinic/hinic_devlink.c >>>> b/drivers/net/ethernet/huawei/hinic/hinic_devlink.c >>>> index c6adc776f3c8..1ec88ebf81d6 100644 >>>> --- a/drivers/net/ethernet/huawei/hinic/hinic_devlink.c >>>> +++ b/drivers/net/ethernet/huawei/hinic/hinic_devlink.c >>>> @@ -342,9 +342,9 @@ static int chip_fault_show(struct devlink_fmsg *fmsg, >>>> >>>>level = event->event.chip.err_level; >>>>if (level < FAULT_LEVEL_MAX) >>>> - strncpy(level_str, fault_level[level], >>>> strlen(fault_level[level])); >>>> + strncpy(level_str, fault_level[level], >>>> strlen(fault_level[level]) + 1); >>> >>> Have you even considered what that code is actually doing? > ... >> I'm sorry that I haven't got what you mean and I haven't found any defects >> in that code. Can you explain more to me? > > David is trying to express the same thing I was trying to explain to > you, you should use sizeof(level_str) as the third argument because > the code is trying to make sure that the destination buffer is not > overrun. > > If you use the strlen() of the source buffer, the strncpy() can still > overflow the destination buffer. > > Now do you understand? > . > Thanks for your explanation. I explained that why I didn't use sizeof(level_str) as the third argument in my previous reply e-mail to you. Because using sizeof(level_str) as the third argument will still cause the following compile warning: In function ‘strncpy’, inlined from ‘chip_fault_show’ at drivers/net/ethernet/huawei/hinic/hinic_devlink.c:345:3: ./include/linux/string.h:297:30: warning: ‘__builtin_strncpy’ specified bound 17 equals destination size [-Wstringop-truncation] 297 | #define __underlying_strncpy __builtin_strncpy Now I know that using strncpy() on NUL-terminated strings is deprecated as Kees Cook points out and actually there is no need to use it in my code.
Re: [PATCH net-next v1] hinic: fix strncpy output truncated compile warnings
On 2020/8/7 17:32, David Laight wrote: > From: Luo bin >> Sent: 07 August 2020 03:09 >> >> fix the compile warnings of 'strncpy' output truncated before >> terminating nul copying N bytes from a string of the same length >> >> Signed-off-by: Luo bin >> Reported-by: kernel test robot >> --- >> V0~V1: >> - use the strlen()+1 pattern consistently >> >> drivers/net/ethernet/huawei/hinic/hinic_devlink.c | 8 >> 1 file changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/net/ethernet/huawei/hinic/hinic_devlink.c >> b/drivers/net/ethernet/huawei/hinic/hinic_devlink.c >> index c6adc776f3c8..1ec88ebf81d6 100644 >> --- a/drivers/net/ethernet/huawei/hinic/hinic_devlink.c >> +++ b/drivers/net/ethernet/huawei/hinic/hinic_devlink.c >> @@ -342,9 +342,9 @@ static int chip_fault_show(struct devlink_fmsg *fmsg, >> >> level = event->event.chip.err_level; >> if (level < FAULT_LEVEL_MAX) >> -strncpy(level_str, fault_level[level], >> strlen(fault_level[level])); >> +strncpy(level_str, fault_level[level], >> strlen(fault_level[level]) + 1); > > Have you even considered what that code is actually doing? > > David > > - > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 > 1PT, UK > Registration No: 1397386 (Wales) > > . > I'm sorry that I haven't got what you mean and I haven't found any defects in that code. Can you explain more to me?
Re: [PATCH net-next] hinic: fix strncpy output truncated compile warnings
On 2020/8/7 8:57, luobin (L) wrote:
> On 2020/8/7 3:01, David Miller wrote:
>> From: Luo bin
>> Date: Thu, 6 Aug 2020 15:48:30 +0800
>>
>>> diff --git a/drivers/net/ethernet/huawei/hinic/hinic_devlink.c
>>> b/drivers/net/ethernet/huawei/hinic/hinic_devlink.c
>>> index c6adc776f3c8..1dc948c07b94 100644
>>> --- a/drivers/net/ethernet/huawei/hinic/hinic_devlink.c
>>> +++ b/drivers/net/ethernet/huawei/hinic/hinic_devlink.c
>>> @@ -342,9 +342,9 @@ static int chip_fault_show(struct devlink_fmsg *fmsg,
>>>
>>> level = event->event.chip.err_level;
>>> if (level < FAULT_LEVEL_MAX)
>>> - strncpy(level_str, fault_level[level],
>>> strlen(fault_level[level]));
>>> + strncpy(level_str, fault_level[level],
>>> strlen(fault_level[level]) + 1);
>>> else
>>> - strncpy(level_str, "Unknown", strlen("Unknown"));
>>> + strncpy(level_str, "Unknown", sizeof(level_str));
>>>
>>> if (level == FAULT_LEVEL_SERIOUS_FLR) {
>>
>> Please fix these cases consistently, either use the strlen()+1 pattern
>> or the "sizeof(destination)" one.
>>
>> Probably sizeof(destination) is best.
>> .
>>
> Will fix. Thanks. Level_str array is initialized to zero, so can't use the
> strlen()+1 pattern, I'll
> use strlen()+1 consistently.
>
I have tried to use 'sizeof(level_str)' instead of 'strlen(fault_level[level])
+ 1', but this will lead
to following compile warning:
In function ‘strncpy’,
inlined from ‘chip_fault_show’ at
drivers/net/ethernet/huawei/hinic/hinic_devlink.c:345:3:
./include/linux/string.h:297:30: warning: ‘__builtin_strncpy’ specified bound
17 equals destination size [-Wstringop-truncation]
297 | #define __underlying_strncpy __builtin_strncpy
So I will use the strlen()+1 pattern consistently.
Re: [PATCH net-next] hinic: fix strncpy output truncated compile warnings
On 2020/8/7 3:01, David Miller wrote:
> From: Luo bin
> Date: Thu, 6 Aug 2020 15:48:30 +0800
>
>> diff --git a/drivers/net/ethernet/huawei/hinic/hinic_devlink.c
>> b/drivers/net/ethernet/huawei/hinic/hinic_devlink.c
>> index c6adc776f3c8..1dc948c07b94 100644
>> --- a/drivers/net/ethernet/huawei/hinic/hinic_devlink.c
>> +++ b/drivers/net/ethernet/huawei/hinic/hinic_devlink.c
>> @@ -342,9 +342,9 @@ static int chip_fault_show(struct devlink_fmsg *fmsg,
>>
>> level = event->event.chip.err_level;
>> if (level < FAULT_LEVEL_MAX)
>> -strncpy(level_str, fault_level[level],
>> strlen(fault_level[level]));
>> +strncpy(level_str, fault_level[level],
>> strlen(fault_level[level]) + 1);
>> else
>> -strncpy(level_str, "Unknown", strlen("Unknown"));
>> +strncpy(level_str, "Unknown", sizeof(level_str));
>>
>> if (level == FAULT_LEVEL_SERIOUS_FLR) {
>
> Please fix these cases consistently, either use the strlen()+1 pattern
> or the "sizeof(destination)" one.
>
> Probably sizeof(destination) is best.
> .
>
Will fix. Thanks. Level_str array is initialized to zero, so can't use the
strlen()+1 pattern, I'll
use strlen()+1 consistently.
Re: [PATCH net-next v3 1/2] hinic: add generating mailbox random index support
On 2020/8/4 6:05, Jakub Kicinski wrote:
> On Sat, 1 Aug 2020 10:49:34 +0800 Luo bin wrote:
>> add support to generate mailbox random id of VF to ensure that
>> mailbox messages PF received are from the correct VF.
>>
>> Signed-off-by: Luo bin
>> ---
>> V2~V3 fix review opinions pointed out by Jakub
>
> In the future please specify what was changed, e.g.:
>
> - use get_random_u32()
> - remove unnecessary parenthesis
>
> etc.
>
Okay, I'll pay attention to that next time.
>> +int hinic_vf_mbox_random_id_init(struct hinic_hwdev *hwdev)
>> +{
>> +u8 vf_in_pf;
>> +int err = 0;
>> +
>> +if (HINIC_IS_VF(hwdev->hwif))
>> +return 0;
>> +
>> +for (vf_in_pf = 1; vf_in_pf <= hwdev->nic_cap.max_vf; vf_in_pf++) {
>> +err = set_vf_mbox_random_id(hwdev, hinic_glb_pf_vf_offset
>> +(hwdev->hwif) + vf_in_pf);
>
> I'm sorry but you must put the call to hinic_glb_pf_vf_offset() on a
> separate line. Perhaps take this call out of the for loop entirely?
>
> The way it's written with the parenthesis on the next line is hard to
> read.
Will fix. Thanks. Taking it out of the loop is a better way to avoid a long
line length.
>
>> +if (err)
>> +break;
>> +}
> .
>
Re: [PATCH net-next v2 1/2] hinic: add generating mailbox random index support
On 2020/8/1 3:52, Jakub Kicinski wrote:
> On Fri, 31 Jul 2020 09:56:41 +0800 Luo bin wrote:
>> add support to generate mailbox random id of VF to ensure that
>> mailbox messages PF received are from the correct VF.
>>
>> Signed-off-by: Luo bin
>
>> diff --git a/drivers/net/ethernet/huawei/hinic/hinic_hw_mbox.c
>> b/drivers/net/ethernet/huawei/hinic/hinic_hw_mbox.c
>> index 47c93f946b94..c72aa8e8bce8 100644
>> --- a/drivers/net/ethernet/huawei/hinic/hinic_hw_mbox.c
>> +++ b/drivers/net/ethernet/huawei/hinic/hinic_hw_mbox.c
>> @@ -486,6 +486,111 @@ static void recv_mbox_handler(struct
>> hinic_mbox_func_to_func *func_to_func,
>> kfree(rcv_mbox_temp);
>> }
>>
>> +static int set_vf_mbox_random_id(struct hinic_hwdev *hwdev, u16 func_id)
>> +{
>> +struct hinic_mbox_func_to_func *func_to_func = hwdev->func_to_func;
>> +struct hinic_set_random_id rand_info = {0};
>> +u16 out_size = sizeof(rand_info);
>> +struct hinic_pfhwdev *pfhwdev;
>> +int ret;
>> +
>> +pfhwdev = container_of(hwdev, struct hinic_pfhwdev, hwdev);
>> +
>> +rand_info.version = HINIC_CMD_VER_FUNC_ID;
>> +rand_info.func_idx = func_id;
>> +rand_info.vf_in_pf = (u8)(func_id -
>> hinic_glb_pf_vf_offset(hwdev->hwif));
>
> this cast is unnecessary
>
Will fix. Thanks for your review.
>> +get_random_bytes(_info.random_id, sizeof(u32));
>
> get_random_u32()
>
Will fix. Thanks for your review.
>> +
>> +func_to_func->vf_mbx_rand_id[func_id] = rand_info.random_id;
>> +
>> +ret = hinic_msg_to_mgmt(>pf_to_mgmt, HINIC_MOD_COMM,
>> +HINIC_MGMT_CMD_SET_VF_RANDOM_ID,
>> +_info, sizeof(rand_info),
>> +_info, _size, HINIC_MGMT_MSG_SYNC);
>> +if ((rand_info.status != HINIC_MGMT_CMD_UNSUPPORTED &&
>> + rand_info.status) || !out_size || ret) {
>> +dev_err(>hwif->pdev->dev, "Set VF random id failed, err:
>> %d, status: 0x%x, out size: 0x%x\n",
>> +ret, rand_info.status, out_size);
>> +return -EIO;
>> +}
>> +
>> +if (rand_info.status == HINIC_MGMT_CMD_UNSUPPORTED)
>> +return rand_info.status;
>> +
>> +func_to_func->vf_mbx_old_rand_id[func_id] =
>> +func_to_func->vf_mbx_rand_id[func_id];
>> +
>> +return 0;
>> +}
>
>> +static bool check_vf_mbox_random_id(struct hinic_mbox_func_to_func
>> *func_to_func,
>> +u8 *header)
>> +{
>> +struct hinic_hwdev *hwdev = func_to_func->hwdev;
>> +struct hinic_mbox_work *mbox_work = NULL;
>> +u64 mbox_header = *((u64 *)header);
>> +u16 offset, src;
>> +u32 random_id;
>> +int vf_in_pf;
>> +
>> +src = HINIC_MBOX_HEADER_GET(mbox_header, SRC_GLB_FUNC_IDX);
>> +
>> +if (IS_PF_OR_PPF_SRC(src) || !func_to_func->support_vf_random)
>> +return true;
>> +
>> +if (!HINIC_IS_PPF(hwdev->hwif)) {
>> +offset = hinic_glb_pf_vf_offset(hwdev->hwif);
>> +vf_in_pf = src - offset;
>> +
>> +if (vf_in_pf < 1 || vf_in_pf > hwdev->nic_cap.max_vf) {
>> +dev_warn(>hwif->pdev->dev,
>> + "Receive vf id(0x%x) is invalid, vf id should
>> be from 0x%x to 0x%x\n",
>> + src, offset + 1,
>> + hwdev->nic_cap.max_vf + offset);
>> +return false;
>> +}
>> +}
>> +
>> +random_id = be32_to_cpu(*(u32 *)(header + MBOX_SEG_LEN +
>> + MBOX_HEADER_SZ));
>> +
>> +if (random_id == func_to_func->vf_mbx_rand_id[src] ||
>> +random_id == func_to_func->vf_mbx_old_rand_id[src])
>
> What guarantees src < MAX_FUNCTION_NUM ?
>
It has been checked if src >= MAX_FUNCTION_NUM in hinic_mbox_func_aeqe_handler
before calling this function.
>> +return true;
>> +
>> +dev_warn(>hwif->pdev->dev,
>> + "The mailbox random id(0x%x) of func_id(0x%x) doesn't match
>> with pf reservation(0x%x)\n",
>> + random_id, src, func_to_func->vf_mbx_rand_id[src]);
>> +
>> +mbox_work = kzalloc(sizeof(*mbox_work), GFP_KERNEL);
>> +if (!mbox_work)
>> +return false;
>> +
>> +mbox_work->func_to_func = func_to_func;
>> +mbox_work->src_func_idx = src;
>> +
>> +INIT_WORK(_work->work, update_random_id_work_handler);
>> +queue_work(func_to_func->workq, _work->work);
>> +
>> +return false;
>> +}
>
>> +int hinic_vf_mbox_random_id_init(struct hinic_hwdev *hwdev)
>> +{
>> +u8 vf_in_pf;
>> +int err = 0;
>> +
>> +if (HINIC_IS_VF(hwdev->hwif))
>> +return 0;
>> +
>> +for (vf_in_pf = 1; vf_in_pf <= hwdev->nic_cap.max_vf; vf_in_pf++) {
>> +err = set_vf_mbox_random_id(hwdev, hinic_glb_pf_vf_offset
>> +(hwdev->hwif) + vf_in_pf);
>
> Parenthesis around hwdev->hwif not necessary
hwdev->hwif is the parameter of
Re: [PATCH net-next v1 1/2] hinic: add generating mailbox random index support
On 2020/7/31 0:25, Jakub Kicinski wrote: > On Thu, 30 Jul 2020 16:37:15 +0800 Luo bin wrote: >> +bool check_vf_mbox_random_id(struct hinic_mbox_func_to_func *func_to_func, >> + u8 *header) > > This set seems to add new W=1 C=1 warnings: > > drivers/net/ethernet/huawei/hinic/hinic_hw_mbox.c:572:6: warning: no previous > prototype for ‘check_vf_mbox_random_id’ [-Wmissing-prototypes] > 572 | bool check_vf_mbox_random_id(struct hinic_mbox_func_to_func > *func_to_func, > | ^~~ > > drivers/net/ethernet/huawei/hinic/hinic_hw_mbox.c:1352:28: warning: symbol > 'hw_cmd_support_vf' was not declared. Should it be static? > . > Will fix. Thanks for your review.
Re: [PATCH net-next] hinic: add generating mailbox random index support
On 2020/7/30 6:03, Jakub Kicinski wrote: > On Wed, 29 Jul 2020 08:59:19 +0800 Luo bin wrote: >> add support to generate mailbox random id for VF to ensure that >> the mailbox message from VF is valid and PF should check whether >> the cmd from VF is supported before passing it to hw. > > This is hard to review. I don't see how the addition of > hinic_mbox_check_cmd_valid() correlates to the random id > thing. Please split this into two or more patches making > one logical change each. > > . > I'll split it into two patches. Thank you!
Re: [PATCH net-next v4 1/2] hinic: add support to handle hw abnormal event
On 2020/7/25 8:04, David Miller wrote:
> From: Luo bin
> Date: Fri, 24 Jul 2020 17:17:31 +0800
>
>> +static int hinic_fw_reporter_dump(struct devlink_health_reporter *reporter,
>> + struct devlink_fmsg *fmsg, void *priv_ctx,
>> + struct netlink_ext_ack *extack)
>> +{
>> +int err;
>> +
>> +if (priv_ctx) {
>> +err = mgmt_watchdog_report_show(fmsg, priv_ctx);
>> +if (err)
>> +return err;
>> +}
>> +
>> +return 0;
>> +}
>
> As Edward Cree pointed out for v3 of this patch series, this 'err' is not
> necessary at all.
> .
>
Will fix. Thanks.
Re: [PATCH net-next v3 1/2] hinic: add support to handle hw abnormal event
On 2020/7/24 17:57, Edward Cree wrote:
> On 23/07/2020 20:08, David Miller wrote:
>> From: Luo bin
>> Date: Thu, 23 Jul 2020 22:40:37 +0800
>>
>>> +static int hinic_fw_reporter_dump(struct devlink_health_reporter *reporter,
>>> + struct devlink_fmsg *fmsg, void *priv_ctx,
>>> + struct netlink_ext_ack *extack)
>>> +{
>>> + struct hinic_mgmt_watchdog_info *watchdog_info;
>>> + int err;
>>> +
>>> + if (priv_ctx) {
>>> + watchdog_info = priv_ctx;
>>> + err = mgmt_watchdog_report_show(fmsg, watchdog_info);
>>> + if (err)
>>> + return err;
>>> + }
>>> +
>>> + return 0;
>>> +}
>> This 'watchdog_info' variable is completely unnecessary, just pass
>> 'priv_ctx' as-is into mgmt_watchdog_report_show().
> Looks like the 'err' variable is unnecessary too...
>
> -ed
> .
>
Will fix. Thanks for your review.
Re: [PATCH net-next v3 1/2] hinic: add support to handle hw abnormal event
On 2020/7/24 3:08, David Miller wrote:
> From: Luo bin
> Date: Thu, 23 Jul 2020 22:40:37 +0800
>
>> +static int hinic_fw_reporter_dump(struct devlink_health_reporter *reporter,
>> + struct devlink_fmsg *fmsg, void *priv_ctx,
>> + struct netlink_ext_ack *extack)
>> +{
>> +struct hinic_mgmt_watchdog_info *watchdog_info;
>> +int err;
>> +
>> +if (priv_ctx) {
>> +watchdog_info = priv_ctx;
>> +err = mgmt_watchdog_report_show(fmsg, watchdog_info);
>> +if (err)
>> +return err;
>> +}
>> +
>> +return 0;
>> +}
>
> This 'watchdog_info' variable is completely unnecessary, just pass
> 'priv_ctx' as-is into mgmt_watchdog_report_show().
> .
>
Will fix. Thanks for your review.
Re: [PATCH net-next v2 1/2] hinic: add support to handle hw abnormal event
On 2020/7/21 2:13, Jakub Kicinski wrote: > On Sat, 18 Jul 2020 16:58:29 +0800 Luo bin wrote: >> add support to handle hw abnormal event such as hardware failure, >> cable unplugged,link error >> >> Signed-off-by: Luo bin >> --- >> V1~V2: add link extended state >> V0~V1: fix auto build test WARNING > > I don't understand what you think devlink health is missing. > > If it is really missing some functionality you require you have to work > on extending it. > > Dumping error event state into kernel logs when we have specific > infrastructure built to address this sort of needs won't fly. > . > Okay, I'll try to implement these functionality based on devlink health.
Re: [PATCH net-next v1 1/2] hinic: add support to handle hw abnormal event
On 2020/7/18 3:11, Jakub Kicinski wrote:
> On Fri, 17 Jul 2020 16:34:47 +0800 Luo bin wrote:
>> add support to handle hw abnormal event such as hardware failure,
>> cable unplugged,link error
>>
>> Signed-off-by: Luo bin
>> Reported-by: kernel test robot
>
>> +static void hinic_comm_recv_mgmt_self_cmd_reg(struct hinic_pfhwdev *pfhwdev,
>> + u8 cmd,
>> + comm_mgmt_self_msg_proc proc)
>> +{
>> +u8 cmd_idx;
>> +
>> +cmd_idx = pfhwdev->proc.cmd_num;
>> +if (cmd_idx >= HINIC_COMM_SELF_CMD_MAX) {
>> +dev_err(>hwdev.hwif->pdev->dev,
>> +"Register recv mgmt process failed, cmd: 0x%x\n", cmd);
>> +return;
>> +}
>> +
>> +pfhwdev->proc.info[cmd_idx].cmd = cmd;
>> +pfhwdev->proc.info[cmd_idx].proc = proc;
>> +pfhwdev->proc.cmd_num++;
>> +}
>> +
>> +static void hinic_comm_recv_mgmt_self_cmd_unreg(struct hinic_pfhwdev
>> *pfhwdev,
>> +u8 cmd)
>> +{
>> +u8 cmd_idx;
>> +
>> +cmd_idx = pfhwdev->proc.cmd_num;
>> +if (cmd_idx >= HINIC_COMM_SELF_CMD_MAX) {
>> +dev_err(>hwdev.hwif->pdev->dev, "Unregister recv mgmt
>> process failed, cmd: 0x%x\n",
>> +cmd);
>> +return;
>> +}
>> +
>> +for (cmd_idx = 0; cmd_idx < HINIC_COMM_SELF_CMD_MAX; cmd_idx++) {
>> +if (cmd == pfhwdev->proc.info[cmd_idx].cmd) {
>> +pfhwdev->proc.info[cmd_idx].cmd = 0;
>> +pfhwdev->proc.info[cmd_idx].proc = NULL;
>> +pfhwdev->proc.cmd_num--;
>> +}
>> +}
>> +}
>> +
>> +static void comm_mgmt_msg_handler(void *handle, u8 cmd, void *buf_in,
>> + u16 in_size, void *buf_out, u16 *out_size)
>> +{
>> +struct hinic_pfhwdev *pfhwdev = handle;
>> +u8 cmd_idx;
>> +
>> +for (cmd_idx = 0; cmd_idx < pfhwdev->proc.cmd_num; cmd_idx++) {
>> +if (cmd == pfhwdev->proc.info[cmd_idx].cmd) {
>> +if (!pfhwdev->proc.info[cmd_idx].proc) {
>> +dev_warn(>hwdev.hwif->pdev->dev,
>> + "PF recv mgmt comm msg handle null,
>> cmd: 0x%x\n",
>> + cmd);
>> +} else {
>> +pfhwdev->proc.info[cmd_idx].proc
>> +(>hwdev, buf_in, in_size,
>> + buf_out, out_size);
>> +}
>> +
>> +return;
>> +}
>> +}
>> +
>> +dev_warn(>hwdev.hwif->pdev->dev, "Received unknown mgmt cpu
>> event: 0x%x\n",
>> + cmd);
>> +
>> +*out_size = 0;
>> +}
>> +
>> +static void chip_fault_show(struct hinic_hwdev *hwdev,
>> +struct hinic_fault_event *event)
>> +{
>> +char fault_level[FAULT_TYPE_MAX][FAULT_SHOW_STR_LEN + 1] = {
>> +"fatal", "reset", "flr", "general", "suggestion"};
>> +char level_str[FAULT_SHOW_STR_LEN + 1] = {0};
>> +u8 level;
>> +
>> +level = event->event.chip.err_level;
>> +if (level < FAULT_LEVEL_MAX)
>> +strncpy(level_str, fault_level[level],
>> +FAULT_SHOW_STR_LEN);
>> +else
>> +strncpy(level_str, "Unknown", FAULT_SHOW_STR_LEN);
>> +
>> +if (level == FAULT_LEVEL_SERIOUS_FLR)
>> +dev_err(>hwif->pdev->dev, "err_level: %d [%s], flr
>> func_id: %d\n",
>> +level, level_str, event->event.chip.func_id);
>> +
>> +dev_err(>hwif->pdev->dev, "Module_id: 0x%x, err_type: 0x%x,
>> err_level: %d[%s], err_csr_addr: 0x%08x, err_csr_value: 0x%08x\n",
>> +event->event.chip.node_id,
>> +event->event.chip.err_type, level, level_str,
>> +event->event.chip.err_csr_addr,
>> +event->event.chip.err_csr_value);
>> +}
>> +
>> +static void fault_report_show(struct hinic_hwdev *hwdev,
>> + struct hinic_fault_event *event)
>> +{
>> +char fault_type[FAULT_TYPE_MAX][FAULT_SHOW_STR_LEN + 1] = {
>> +"chip", "ucode", "mem rd timeout", "mem wr timeout",
>> +"reg rd timeout", "reg wr timeout", "phy fault"};
>> +char type_str[FAULT_SHOW_STR_LEN + 1] = {0};
>> +
>> +dev_err(>hwif->pdev->dev, "Fault event report received, func_id:
>> %d\n",
>> +HINIC_HWIF_FUNC_IDX(hwdev->hwif));
>> +
>> +if (event->type < FAULT_TYPE_MAX)
>> +strncpy(type_str, fault_type[event->type], FAULT_SHOW_STR_LEN);
>> +else
>> +strncpy(type_str, "Unknown", FAULT_SHOW_STR_LEN);
>> +
>> +dev_err(>hwif->pdev->dev, "Fault type: %d [%s]\n", event->type,
>> +type_str);
>> +dev_err(>hwif->pdev->dev, "Fault val[0]: 0x%08x, val[1]:
>> 0x%08x, val[2]: 0x%08x, val[3]: 0x%08x\n",
>> +event->event.val[0], event->event.val[1], event->event.val[2],
>> +
Re: [PATCH net-next 2/2] hinic: add log in exception handling processes
On 2020/7/16 23:27, Jakub Kicinski wrote: > On Thu, 16 Jul 2020 20:50:56 +0800 Luo bin wrote: >> improve the error message when functions return failure and dump >> relevant registers in some exception handling processes >> >> Signed-off-by: Luo bin > > For kernel builds with W=1 C=1 flags this patch adds 12 warnings to > drivers/net/ethernet/huawei/hinic/hinic_hw_eqs.c and 39 warnings to > drivers/net/ethernet/huawei/hinic/hinic_hw_if.h. > > It seems like you're missing byte swaps. > . > Will fix. Thank you!
Re: [PATCH net-next v2] hinic: add firmware update support
On 2020/7/15 2:37, Jakub Kicinski wrote:
> On Tue, 14 Jul 2020 20:54:33 +0800 Luo bin wrote:
>> add support to update firmware by the devlink flashing API
>>
>> Signed-off-by: Luo bin
>
> Minor nits below, otherwise I think this looks good.
>
>> +static int hinic_firmware_update(struct hinic_devlink_priv *priv,
>> + const struct firmware *fw)
>> +{
>> +struct host_image_st host_image;
>> +int err;
>> +
>> +memset(_image, 0, sizeof(struct host_image_st));
>> +
>> +if (!check_image_valid(priv, fw->data, fw->size, _image) ||
>> +!check_image_integrity(priv, _image, FW_UPDATE_COLD) ||
>> +!check_image_device_type(priv, host_image.device_id))
>
> These helpers should also set an appropriate message in extack, so the
> user can see it on the command line / inside their application.
>
>> +return -EINVAL;
>> +
>> +dev_info(>hwdev->hwif->pdev->dev, "Flash firmware begin\n");
>> +
>> +err = hinic_flash_fw(priv, fw->data, _image);
>> +if (err) {
>> +if (err == HINIC_FW_DISMATCH_ERROR)
>> +dev_err(>hwdev->hwif->pdev->dev, "Firmware image
>> doesn't match this card, please use newer image, err: %d\n",
>
> Here as well - please make sure to return an error messages through
> extack.
>
>> +err);
>> +else
>> +dev_err(>hwdev->hwif->pdev->dev, "Send firmware
>> image data failed, err: %d\n",
>> +err);
>> +return err;
>> +}
>> +
>> +dev_info(>hwdev->hwif->pdev->dev, "Flash firmware end\n");
>> +
>> +return 0;
>> +}
>
>> @@ -1086,6 +1090,17 @@ static int nic_dev_init(struct pci_dev *pdev)
>> return PTR_ERR(hwdev);
>> }
>>
>> +devlink = hinic_devlink_alloc();
>> +if (!devlink) {
>> +dev_err(>dev, "Hinic devlink alloc failed\n");
>> +err = -ENOMEM;
>> +goto err_devlink_alloc;
>> +}
>> +
>> +priv = devlink_priv(devlink);
>> +priv->hwdev = hwdev;
>> +priv->devlink = devlink;
>
> No need to remember the devlink pointer here, you can use
> priv_to_devlink(priv) to go from priv to devlink.
>
>> +
>> num_qps = hinic_hwdev_num_qps(hwdev);
>> if (num_qps <= 0) {
>> dev_err(>dev, "Invalid number of QPS\n");
> .
>
Will fix all. Thanks for your review.
Re: [PATCH net-next v1] hinic: add firmware update support
On 2020/7/14 6:42, Jakub Kicinski wrote:
> On Mon, 13 Jul 2020 22:05:22 +0800 Luo bin wrote:
>> add support to update firmware by the devlink flashing API
>>
>> Signed-off-by: Luo bin
>> ---
>> V0~V1: remove the implementation from ethtool to devlink
>
> Thanks!
>
>> +static int check_image_device_type(struct hinic_dev *nic_dev,
>> + u32 image_device_type)
>> +{
>> +struct hinic_comm_board_info board_info = {0};
>> +
>> +if (image_device_type) {
>> +if (!hinic_get_board_info(nic_dev->hwdev, _info)) {
>> +if (image_device_type == board_info.info.board_type)
>> +return true;
>
> Please simplify this, you shouldn't need more than 1 indentation level
> here.
>
>> +dev_err(_dev->hwdev->hwif->pdev->dev, "The device
>> type of upgrade file doesn't match the device type of current firmware,
>> please check the upgrade file\n");
>> +dev_err(_dev->hwdev->hwif->pdev->dev, "The image
>> device type: 0x%x, firmware device type: 0x%x\n",
>> +image_device_type, board_info.info.board_type);
>> +
>> +return false;
>> +}
>> +}
>> +
>> +return false;
>> +}
>> +
>> +static int hinic_flash_fw(struct hinic_dev *nic_dev, const u8 *data,
>> + struct host_image_st *host_image, u32 boot_flag)
>
> You seem to always pass boot_flag = 0, AFAICT, please remove the
> parameter and related code.
>
>> +{
>> +u32 section_remain_send_len, send_fragment_len, send_pos, up_total_len;
>> +struct hinic_cmd_update_fw *fw_update_msg = NULL;
>> +u32 section_type, section_crc, section_version;
>> +u32 i, len, section_len, section_offset;
>> +u16 out_size = sizeof(*fw_update_msg);
>> +int total_len_flag = 0;
>> +int err;
>
>> +int hinic_devlink_register(struct devlink *devlink, struct device *dev)
>> +{
>> +int err;
>> +
>> +err = devlink_register(devlink, dev);
>> +
>> +return err;
>
> No need for temporary variable.
>
>> +}
>> +
>> +void hinic_devlink_unregister(struct devlink *devlink)
>> +{
>> +devlink_unregister(devlink);
>> +}
>
>> diff --git a/drivers/net/ethernet/huawei/hinic/hinic_main.c
>> b/drivers/net/ethernet/huawei/hinic/hinic_main.c
>> index 834a20a0043c..1dfa09411590 100644
>> --- a/drivers/net/ethernet/huawei/hinic/hinic_main.c
>> +++ b/drivers/net/ethernet/huawei/hinic/hinic_main.c
>> @@ -18,6 +18,7 @@
>> #include
>> #include
>> #include
>> +#include
>> #include
>> #include
>> #include
>> @@ -25,6 +26,7 @@
>>
>> #include "hinic_hw_qp.h"
>> #include "hinic_hw_dev.h"
>> +#include "hinic_devlink.h"
>> #include "hinic_port.h"
>> #include "hinic_tx.h"
>> #include "hinic_rx.h"
>> @@ -1075,9 +1077,11 @@ static int nic_dev_init(struct pci_dev *pdev)
>> struct hinic_rx_mode_work *rx_mode_work;
>> struct hinic_txq_stats *tx_stats;
>> struct hinic_rxq_stats *rx_stats;
>> +struct hinic_dev *devlink_dev;
>> struct hinic_dev *nic_dev;
>> struct net_device *netdev;
>> struct hinic_hwdev *hwdev;
>> +struct devlink *devlink;
>> int err, num_qps;
>>
>> hwdev = hinic_init_hwdev(pdev);
>> @@ -1086,6 +1090,15 @@ static int nic_dev_init(struct pci_dev *pdev)
>> return PTR_ERR(hwdev);
>> }
>>
>> +devlink = hinic_devlink_alloc();
>> +if (!devlink) {
>> +dev_err(>dev, "Hinic devlink alloc failed\n");
>> +err = -ENOMEM;
>> +goto err_devlink_alloc;
>> +}
>> +
>> +devlink_dev = devlink_priv(devlink);
>> +
>> num_qps = hinic_hwdev_num_qps(hwdev);
>> if (num_qps <= 0) {
>> dev_err(>dev, "Invalid number of QPS\n");
>> @@ -1121,6 +1134,7 @@ static int nic_dev_init(struct pci_dev *pdev)
>> nic_dev->sriov_info.hwdev = hwdev;
>> nic_dev->sriov_info.pdev = pdev;
>> nic_dev->max_qps = num_qps;
>> +nic_dev->devlink = devlink;
>>
>> hinic_set_ethtool_ops(netdev);
>>
>> @@ -1146,6 +1160,11 @@ static int nic_dev_init(struct pci_dev *pdev)
>> goto err_workq;
>> }
>>
>> +memcpy(devlink_dev, nic_dev, sizeof(*nic_dev));
>
> Please create a separate structure for the devlink-level priv.
> This doesn't look right.
>
>> +err = hinic_devlink_register(devlink, >dev);
>> +if (err)
>> +goto err_devlink_reg;
>> +
>> pci_set_drvdata(pdev, netdev);
>>
>> err = hinic_port_get_mac(nic_dev, netdev->dev_addr);
>
> .
>
Will fix all. Thanks for your review.
Re: [PATCH net-next] hinic: add firmware update support
On 2020/7/7 0:57, Jakub Kicinski wrote: > On Mon, 6 Jul 2020 22:54:06 +0800 Luo bin wrote: >> add support to update firmware with with "ethtool -f" cmd >> >> Signed-off-by: Luo bin > > drivers/net/ethernet/huawei/hinic/hinic_ethtool.c:1996:44: warning: missing > braces around initializer > drivers/net/ethernet/huawei/hinic/hinic_ethtool.c:1996:44: warning: missing > braces around initializer > > But really - please try to implement the devlink flashing API, using > ethtool for this is deprecated. > . > Okay. Will fix. Thanks for your review.
Re: [PATCH net] hinic: fix passing non negative value to ERR_PTR
On 2020/7/1 0:20, Jakub Kicinski wrote:
> On Tue, 30 Jun 2020 14:35:54 +0800 Luo bin wrote:
>> get_dev_cap and set_resources_state functions may return a positive
>> value because of hardware failure, and the positive return value
>> can not be passed to ERR_PTR directly.
>>
>> Fixes: 7dd29ee12865 ("net-next/hinic: add sriov feature support")
>> Signed-off-by: Luo bin
>
> Fixes tag: Fixes: 7dd29ee12865 ("net-next/hinic: add sriov feature support")
> Has these problem(s):
> - Subject does not match target commit subject
> Just use
> git log -1 --format='Fixes: %h ("%s")'
> .
>
Will fix. Thanks.
Re: [PATCH net-next v3 0/5] hinic: add some ethtool ops support
On 2020/6/28 8:52, David Miller wrote: > From: Luo bin > Date: Sat, 27 Jun 2020 14:52:37 +0800 > >> patch #1: support to set and get pause params with >> "ethtool -A/a" cmd >> patch #2: support to set and get irq coalesce params with >> "ethtool -C/c" cmd >> patch #3: support to do self test with "ethtool -t" cmd >> patch #4: support to identify physical device with "ethtool -p" cmd >> patch #5: support to get eeprom information with "ethtool -m" cmd > > In general, I want you to decrease the amount of log messages. > > You should only use them when the device or the kernel does something > unexpected which should be notifier to the user. > > Kernel log messages are not for informating the user of limitations > of what they can perform with "ethtool". > > For example, when setting pause paramenters, you complain in the logs > if the autonet setting is different. > > This is completely inappropriate. > > Then in patch #2 you have these crazy macros that print out state > changes with netdev_info(). That is also inappropriate. The user > gets a success status, and they can query the settings later if > they like as well. > > Please stop abusing kernel log messaging, it isn't a framework for > giving more detailed ethtool command result statuses. > > Thank you. > . > Okay, I'll remove these normal kernel logs. Thank you for your review.
Re: [PATCH net-next v2 5/5] hinic: add support to get eeprom information
On 2020/6/24 6:02, Jakub Kicinski wrote:
> On Tue, 23 Jun 2020 22:24:09 +0800 Luo bin wrote:
>> +int hinic_get_sfp_type(struct hinic_hwdev *hwdev, u8 *data0, u8 *data1)
>> +{
>> +u8 sfp_data[STD_SFP_INFO_MAX_SIZE];
>> +u16 len;
>> +int err;
>> +
>> +if (!hwdev || !data0 || !data1)
>> +return -EINVAL;
>
> No need to check these, callers are correct. We don't do defensive
> programming in the kernel.
>
Will fix. Thank you for your review.
>> +return 0;
>
> double space
Will fix. Thank you for your review.
Re: [PATCH net-next v2 1/5] hinic: add support to set and get pause params
On 2020/6/24 5:54, Jakub Kicinski wrote:
> On Tue, 23 Jun 2020 22:24:05 +0800 Luo bin wrote:
>> diff --git a/drivers/net/ethernet/huawei/hinic/hinic_main.c
>> b/drivers/net/ethernet/huawei/hinic/hinic_main.c
>> index e9e6f4c9309a..e69edb01fd9b 100644
>> --- a/drivers/net/ethernet/huawei/hinic/hinic_main.c
>> +++ b/drivers/net/ethernet/huawei/hinic/hinic_main.c
>> @@ -467,6 +467,7 @@ int hinic_open(struct net_device *netdev)
>> if (ret)
>> netif_warn(nic_dev, drv, netdev,
>> "Failed to revert port state\n");
>> +
>
> Unrelated chunk, please drop.
>
Will undo this.Thanks.
>> err_port_state:
>> free_rxqs(nic_dev);
>> if (nic_dev->flags & HINIC_RSS_ENABLE) {
>> @@ -887,6 +888,26 @@ static void netdev_features_init(struct net_device
>> *netdev)
>> netdev->features = netdev->hw_features | NETIF_F_HW_VLAN_CTAG_FILTER;
>> }
>>
>> +static void hinic_refresh_nic_cfg(struct hinic_dev *nic_dev)
>> +{
>> +struct hinic_nic_cfg *nic_cfg = _dev->hwdev->func_to_io.nic_cfg;
>> +struct hinic_pause_config pause_info = {0};
>> +struct hinic_port_cap port_cap = {0};
>> +
>> +if (hinic_port_get_cap(nic_dev, _cap))
>> +return;
>> +
>> +mutex_lock(_cfg->cfg_mutex);
>> +if (nic_cfg->pause_set || !port_cap.autoneg_state) {
>> +nic_cfg->auto_neg = port_cap.autoneg_state;
>> +pause_info.auto_neg = nic_cfg->auto_neg;
>> +pause_info.rx_pause = nic_cfg->rx_pause;
>> +pause_info.tx_pause = nic_cfg->tx_pause;
>> +hinic_set_hw_pause_info(nic_dev->hwdev, _info);
>> +}
>> +mutex_unlock(_cfg->cfg_mutex);
>> +}
>> +
>> /**
>> * link_status_event_handler - link event handler
>> * @handle: nic device for the handler
>> @@ -918,6 +939,9 @@ static void link_status_event_handler(void *handle, void
>> *buf_in, u16 in_size,
>>
>> up(_dev->mgmt_lock);
>>
>> +if (!HINIC_IS_VF(nic_dev->hwdev->hwif))
>> +hinic_refresh_nic_cfg(nic_dev);
>> +
>> netif_info(nic_dev, drv, nic_dev->netdev, "HINIC_Link is UP\n");
>> } else {
>> down(_dev->mgmt_lock);
>> @@ -950,26 +974,38 @@ static int set_features(struct hinic_dev *nic_dev,
>> u32 csum_en = HINIC_RX_CSUM_OFFLOAD_EN;
>> int err = 0;
>>
>> -if (changed & NETIF_F_TSO)
>> +if (changed & NETIF_F_TSO) {
>> err = hinic_port_set_tso(nic_dev, (features & NETIF_F_TSO) ?
>> HINIC_TSO_ENABLE : HINIC_TSO_DISABLE);
>> +if (err)
>> +return err;
>> +}
>>
>> -if (changed & NETIF_F_RXCSUM)
>> +if (changed & NETIF_F_RXCSUM) {
>> err = hinic_set_rx_csum_offload(nic_dev, csum_en);
>> +if (err)
>> +return err;
>> +}
>>
>> if (changed & NETIF_F_LRO) {
>> err = hinic_set_rx_lro_state(nic_dev,
>> !!(features & NETIF_F_LRO),
>> HINIC_LRO_RX_TIMER_DEFAULT,
>> HINIC_LRO_MAX_WQE_NUM_DEFAULT);
>> +if (err)
>> +return err;
>> }
>>
>> -if (changed & NETIF_F_HW_VLAN_CTAG_RX)
>> +if (changed & NETIF_F_HW_VLAN_CTAG_RX) {
>> err = hinic_set_rx_vlan_offload(nic_dev,
>> !!(features &
>> NETIF_F_HW_VLAN_CTAG_RX));
>> +if (err)
>> +return err;
>> +}
>
> I missed this on v1, but this looks broken, multiple features may be
> changed at the same time. If user requests RXCSUM and LRO to be changed
> and LRO change fails the RXCSUM will be left in a different state than
> dev->features indicates.
>
You're right. Will fix. Thank you.
>> -return err;
>> +/* enable pause and disable pfc by default */
>> +return hinic_dcb_set_pfc(nic_dev->hwdev, 0, 0);
>
> Why do you disable PFC every time features are changed?
>
It can be optimized. Thanks.
>> +int hinic_dcb_set_pfc(struct hinic_hwdev *hwdev, u8 pfc_en, u8 pfc_bitmap)
>
> This is only ever called with 0, 0 as parameters.
> .
>
This function will be called with other parameters before long to support DCB.
So I intend not to modify it.
Re: [PATCH net-next v1 5/5] hinic: add support to get eeprom information
On 2020/6/23 6:15, Jakub Kicinski wrote:
> On Sat, 20 Jun 2020 17:42:58 +0800 Luo bin wrote:
>> diff --git a/drivers/net/ethernet/huawei/hinic/hinic_port.h
>> b/drivers/net/ethernet/huawei/hinic/hinic_port.h
>> index 5c916875f295..0d0354241345 100644
>> --- a/drivers/net/ethernet/huawei/hinic/hinic_port.h
>> +++ b/drivers/net/ethernet/huawei/hinic/hinic_port.h
>> @@ -677,6 +677,37 @@ struct hinic_led_info {
>> u8 reset;
>> };
>>
>> +#define MODULE_TYPE_SFP 0x3
>> +#define MODULE_TYPE_QSFP28 0x11
>> +#define MODULE_TYPE_QSFP0x0C
>> +#define MODULE_TYPE_QSFP_PLUS 0x0D
>> +
>> +#define STD_SFP_INFO_MAX_SIZE 640
>
> Please use the existing defines, e.g. from #include
> there is no need for every driver to redefine those constants.
> .
>
Will fix. Thanks for your review.
Re: [PATCH net-next v1 2/5] hinic: add support to set and get irq coalesce
On 2020/6/23 6:08, Jakub Kicinski wrote:
>> +if (coal->tx_max_coalesced_frames > COALESCE_MAX_PENDING_LIMIT) {
>> +netif_err(nic_dev, drv, netdev,
>> + "Tx_max_coalesced_frames out of range[%d-%d]\n", 0,
>> + COALESCE_MAX_PENDING_LIMIT);
>> +return -EOPNOTSUPP;
>> +}
>> +
>> +return 0;
>> +}
> I think ERANGE is a more appropriate error code in these?
Will fix. Thanks for your review.
Re: [PATCH net-next v1 2/5] hinic: add support to set and get irq coalesce
On 2020/6/23 6:07, Jakub Kicinski wrote:
> On Sat, 20 Jun 2020 17:42:55 +0800 Luo bin wrote:
>> @@ -1144,8 +1190,16 @@ static int nic_dev_init(struct pci_dev *pdev)
>> goto err_reg_netdev;
>> }
>>
>> +err = hinic_init_intr_coalesce(nic_dev);
>> +if (err) {
>> +netif_err(nic_dev, drv, netdev, "Failed to
>> init_intr_coalesce\n");
>> +goto err_init_intr;
>> +}
>> +
>> return 0;
>>
>> +err_init_intr:
>> +unregister_netdev(netdev);
>> err_reg_netdev:
>> err_set_features:
>> hinic_hwdev_cb_unregister(nic_dev->hwdev,
>
> A little suspicious - you should finish all init before device is
> registered, once registered the interface can be immediately brought
> up.
> .
>
Will fix. Thanks for your review.
Re: [PATCH net-next v1 5/5] hinic: add support to get eeprom information
On 2020/6/21 0:00, Andrew Lunn wrote:
>> +static int hinic_get_module_eeprom(struct net_device *netdev,
>> + struct ethtool_eeprom *ee, u8 *data)
>> +{
>> +struct hinic_dev *nic_dev = netdev_priv(netdev);
>> +u8 sfp_data[STD_SFP_INFO_MAX_SIZE];
>
> sfp_data will contain whatever is on the stack.
>
>> +u16 len;
>> +int err;
>> +
>> +if (!ee->len || ((ee->len + ee->offset) > STD_SFP_INFO_MAX_SIZE))
>> +return -EINVAL;
>> +
>> +memset(data, 0, ee->len);
>
> This clears what you are going to return.
>
>> +
>> +err = hinic_get_sfp_eeprom(nic_dev->hwdev, sfp_data, );
>
> Upto len bytes of sfp_data now contain useful data. The rest of
> sfp_data is still stack data.
>
>
>> +if (err)
>> +return err;
>> +
>> +memcpy(data, sfp_data + ee->offset, ee->len);
>
> If len < ee->len, you have just returned to user space some stack data.
>
>Andrew
> .
>
The whole sfp_data will be assigned values in hinic_get_sfp_eeprom function,
so stack data won't be returned to user space. Thanks for your review.
Re: [PATCH net-next 5/5] hinic: add support to get eeprom information
On 2020/6/4 11:01, Jakub Kicinski wrote: > On Wed, 3 Jun 2020 14:20:15 +0800 Luo bin wrote: >> add support to get eeprom information from the plug-in module >> with ethtool -m cmd. >> >> Signed-off-by: Luo bin > > drivers/net/ethernet/huawei/hinic/hinic_port.c:1386:5: warning: variable > port_id set but not used [-Wunused-but-set-variable] > 1386 | u8 port_id; > | ^~~ > . > Will fix. Thanks.
Re: [PATCH net-next v5] hinic: add set_channels ethtool_ops support
On 2020/6/2 1:53, David Miller wrote:
From: Luo bin
Date: Mon, 1 Jun 2020 18:57:48 +0800
@@ -470,6 +470,11 @@ netdev_tx_t hinic_xmit_frame(struct sk_buff *skb, struct
net_device *netdev)
struct hinic_txq *txq;
struct hinic_qp *qp;
+ if (unlikely(!netif_carrier_ok(netdev))) {
+ dev_kfree_skb_any(skb);
+ return NETDEV_TX_OK;
+ }
As stated by another reviewer, this change is unrelated to adding
set_channels support. Please remove it from this patch.
Will fix. Thanks.
.
Re: [PATCH net-next v2] hinic: add set_channels ethtool_ops support
On 2020/5/30 1:44, Jakub Kicinski wrote:
On Thu, 28 May 2020 18:36:33 + Luo bin wrote:
add support to change TX/RX queue number with ethtool -L
Signed-off-by: Luo bin
Luo bin, your patches continue to come with Date: header being in the
past. Also suspiciously no time zone offset. Can you address this?
+static int hinic_set_channels(struct net_device *netdev,
+ struct ethtool_channels *channels)
+{
+ struct hinic_dev *nic_dev = netdev_priv(netdev);
+ unsigned int count = channels->combined_count;
+ int err;
+
+ if (!count) {
+ netif_err(nic_dev, drv, netdev,
+ "Unsupported combined_count: 0\n");
+ return -EINVAL;
+ }
This check has been added to the core since the last version of you
patch:
/* ensure there is at least one RX and one TX channel */
if (!channels.combined_count &&
(!channels.rx_count || !channels.tx_count))
return -EINVAL;
+ netif_info(nic_dev, drv, netdev, "Set max combined queue number from %d to
%d\n",
+ hinic_hwdev_num_qps(nic_dev->hwdev), count);
+
+ if (netif_running(netdev)) {
+ netif_info(nic_dev, drv, netdev, "Restarting netdev\n");
+ hinic_close(netdev);
+
+ nic_dev->hwdev->nic_cap.num_qps = count;
+
+ err = hinic_open(netdev);
+ if (err) {
+ netif_err(nic_dev, drv, netdev,
+ "Failed to open netdev\n");
+ return -EFAULT;
+ }
+ } else {
+ nic_dev->hwdev->nic_cap.num_qps = count;
+ }
+
+ return 0;
}
Will fix. Thanks.
.
Re: [PATCH net-next] hinic: add support to set and get pause param
Will fix. Thanks. On 2020/5/17 4:25, David Miller wrote: From: Luo bin Date: Sat, 16 May 2020 02:00:30 + add support to set pause param with ethtool -A and get pause param with ethtool -a. Also remove set_link_ksettings ops for VF. Signed-off-by: Luo bin Why are you using a semaphore and not a plain mutex. Semaphores should be used as an absolute last resort, and only after trying vigorously to use other locking primitives. .
Re: [PATCH net-next] hinic: add set_channels ethtool_ops support
On 2020/5/16 2:13, Michal Kubecek wrote:
On Fri, May 15, 2020 at 12:35:47AM +, Luo bin wrote:
add support to change TX/RX queue number with ethtool -L
Signed-off-by: Luo bin
---
.../net/ethernet/huawei/hinic/hinic_ethtool.c | 67 +--
.../net/ethernet/huawei/hinic/hinic_hw_dev.c | 7 ++
.../net/ethernet/huawei/hinic/hinic_hw_dev.h | 2 +
.../net/ethernet/huawei/hinic/hinic_main.c| 5 +-
drivers/net/ethernet/huawei/hinic/hinic_tx.c | 5 ++
5 files changed, 79 insertions(+), 7 deletions(-)
diff --git a/drivers/net/ethernet/huawei/hinic/hinic_ethtool.c
b/drivers/net/ethernet/huawei/hinic/hinic_ethtool.c
index ace18d258049..92a0e3bd19c3 100644
--- a/drivers/net/ethernet/huawei/hinic/hinic_ethtool.c
+++ b/drivers/net/ethernet/huawei/hinic/hinic_ethtool.c
@@ -619,14 +619,68 @@ static void hinic_get_channels(struct net_device *netdev,
struct hinic_dev *nic_dev = netdev_priv(netdev);
struct hinic_hwdev *hwdev = nic_dev->hwdev;
- channels->max_rx = hwdev->nic_cap.max_qps;
- channels->max_tx = hwdev->nic_cap.max_qps;
+ channels->max_rx = 0;
+ channels->max_tx = 0;
channels->max_other = 0;
- channels->max_combined = 0;
- channels->rx_count = hinic_hwdev_num_qps(hwdev);
- channels->tx_count = hinic_hwdev_num_qps(hwdev);
+ channels->max_combined = nic_dev->max_qps;
+ channels->rx_count = 0;
+ channels->tx_count = 0;
channels->other_count = 0;
- channels->combined_count = 0;
+ channels->combined_count = hinic_hwdev_num_qps(hwdev);
+}
+
+int hinic_set_channels(struct net_device *netdev,
+ struct ethtool_channels *channels)
+{
+ struct hinic_dev *nic_dev = netdev_priv(netdev);
+ unsigned int count = channels->combined_count;
+ int err;
+
+ if (!count) {
+ netif_err(nic_dev, drv, netdev,
+ "Unsupported combined_count: 0\n");
+ return -EINVAL;
+ }
+
+ if (channels->tx_count || channels->rx_count || channels->other_count) {
+ netif_err(nic_dev, drv, netdev,
+ "Setting rx/tx/other count not supported\n");
+ return -EINVAL;
+ }
With max_* reported as 0, these will be caught in ethnl_set_channels()
or ethtool_set_channels().
---Will fix. Thanks
+ if (!(nic_dev->flags & HINIC_RSS_ENABLE)) {
+ netif_err(nic_dev, drv, netdev,
+ "This function doesn't support RSS, only support 1 queue
pair\n");
+ return -EOPNOTSUPP;
+ }
I'm not sure if the request should fail even if requested count is
actually 1.
+ if (count > nic_dev->max_qps) {
+ netif_err(nic_dev, drv, netdev,
+ "Combined count %d exceeds limit %d\n",
+ count, nic_dev->max_qps);
+ return -EINVAL;
+ }
As above, this check has been already performed in ethnl_set_channels()
or ethtool_set_channels().
---Will fix. Thanks
+ netif_info(nic_dev, drv, netdev, "Set max combined queue number from %d to
%d\n",
+ hinic_hwdev_num_qps(nic_dev->hwdev), count);
We have netlink notifications now, is it necessary to log successful
changes?
---I think it contributes to locating defects for developers.
Michal
.
Re: linux-next: manual merge of the net-next tree with the net tree
On 2020/5/13 0:47, Jakub Kicinski wrote:
On Tue, 12 May 2020 13:30:51 +1000 Stephen Rothwell wrote:
Hi all,
Today's linux-next merge of the net-next tree got conflicts in:
drivers/net/ethernet/huawei/hinic/hinic_hw_mgmt.c
drivers/net/ethernet/huawei/hinic/hinic_main.c
between commit:
e8a1b0efd632 ("hinic: fix a bug of ndo_stop")
from the net tree and commit:
7dd29ee12865 ("hinic: add sriov feature support")
from the net-next tree.
I fixed it up (I think, see below) and can carry the fix as necessary. This
is now fixed as far as linux-next is concerned, but any non trivial
conflicts should be mentioned to your upstream maintainer when your tree
is submitted for merging. You may also want to consider cooperating
with the maintainer of the conflicting tree to minimise any particularly
complex conflicts.
I had a feeling this was gonna happen :(
Resolution looks correct, thank you!
Luo bin, if you want to adjust the timeouts (you had slightly different
ones depending on the command in the first version of the fix) - you can
follow up with a patch to net-next once Dave merges net into net-next
(usually happens every two weeks).
OK. Thanks.
.
Re: [PATCH net v2] hinic: fix a bug of ndo_stop
Will fix. Thanks. On 2020/5/10 6:37, Jakub Kicinski wrote: On Fri, 8 May 2020 20:19:33 + Luo bin wrote: if some function in ndo_stop interface returns failure because of hardware fault, must go on excuting rest steps rather than return failure directly, otherwise will cause memory leak.And bump the timeout for SET_FUNC_STATE to ensure that cmd won't return failure when hw is busy. Otherwise hw may stomp host memory if we free memory regardless of the return value of SET_FUNC_STATE. Signed-off-by: Luo bin Doesn't apply to the net tree: error: patch failed: drivers/net/ethernet/huawei/hinic/hinic_hw_mgmt.c:353 error: drivers/net/ethernet/huawei/hinic/hinic_hw_mgmt.c: patch does not apply error: patch failed: drivers/net/ethernet/huawei/hinic/hinic_main.c:504 error: drivers/net/ethernet/huawei/hinic/hinic_main.c: patch does not apply hint: Use 'git am --show-current-patch' to see the failed patch Applying: hinic: fix a bug of ndo_stop Please also include a Fixes tag when you repost. .
Re: [PATCH net-next v1] hinic: add three net_device_ops of vf
Will fix. Thanks for your review. On 2020/5/9 5:36, Jakub Kicinski wrote: On Thu, 7 May 2020 18:21:19 + Luo bin wrote: + return hinic_msg_to_mgmt(>pf_to_mgmt, HINIC_MOD_COMM, +HINIC_COMM_CMD_HWCTXT_SET, +_ioctxt, sizeof(hw_ioctxt), NULL, +NULL, HINIC_MGMT_MSG_SYNC); + + return 0; Oh, well, I think there will be a v2 :) .
Re: [PATCH net v1] hinic: fix a bug of ndo_stop
The two modified points are relevant. We bump the timeout for SET_FUNC_STATE
to ensure that cmd won't return failure when hw is busy. Otherwise hw
may stomp
host memory if we free memory regardless of the return value of
SET_FUNC_STATE.
I will mention the timeout changes in the commit message. And the bug
exists since
the first commit, not introduced. Thanks for your review.
On 2020/5/9 5:24, Jakub Kicinski wrote:
On Thu, 7 May 2020 18:22:27 + Luo bin wrote:
if some function in ndo_stop interface returns failure because of
hardware fault, must go on excuting rest steps rather than return
failure directly, otherwise will cause memory leak
Signed-off-by: Luo bin
The code looks good, but would it make sense to split this patch into
two? First one that ignores the return values on close path with these
fixes tags:
Fixes: e2585ea77538 ("net-next/hinic: Add Rx handler")
Fixes: c4d06d2d208a ("net-next/hinic: Add Rx mode and link event handler")
And a separate patch which bumps the timeout for SET_FUNC_STATE? Right
now you don't even mention the timeout changes in the commit message.
.
Re: [PATCH net] hinic: fix a bug of ndo_stop
All right，will fix. On 2020/5/8 9:00, David Miller wrote: From: Luo bin Date: Thu, 7 May 2020 04:32:22 + + ulong timeo; Please fully spell out "unsigned long" for this type. The same problem exists in your net-next patch submission as well. Thank you. .
