[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <b83d8d92-44c5-438f-acef-d5781ab44f0d@intel.com>
Date: Wed, 10 Jul 2024 15:59:23 +0200
From: Przemek Kitszel <przemyslaw.kitszel@...el.com>
To: Simon Horman <horms@...nel.org>, Mateusz Polchlopek
<mateusz.polchlopek@...el.com>
CC: <intel-wired-lan@...ts.osuosl.org>, <apw@...onical.com>,
<joe@...ches.com>, <dwaipayanray1@...il.com>, <lukas.bulwahn@...il.com>,
<akpm@...ux-foundation.org>, <willemb@...gle.com>, <edumazet@...gle.com>,
<linux-kernel@...r.kernel.org>, <netdev@...r.kernel.org>, Igor Bagnucki
<igor.bagnucki@...el.com>, Wojciech Drewek <wojciech.drewek@...el.com>
Subject: Re: [Intel-wired-lan] [PATCH iwl-next v1 3/6] ice: add Tx hang
devlink health reporter
On 7/8/24 14:40, Simon Horman wrote:
> On Wed, Jul 03, 2024 at 08:59:19AM -0400, Mateusz Polchlopek wrote:
>> From: Przemek Kitszel <przemyslaw.kitszel@...el.com>
>>
>> Add Tx hang devlink health reporter, see struct ice_tx_hang_event to see
>> what is reported.
>>
>> Subsequent commits will extend it by more info, for now it dumps
>> descriptors with little metadata.
>>
>> Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@...el.com>
>> Reviewed-by: Igor Bagnucki <igor.bagnucki@...el.com>
>> Reviewed-by: Wojciech Drewek <wojciech.drewek@...el.com>
>> Signed-off-by: Mateusz Polchlopek <mateusz.polchlopek@...el.com>
>
> ...
>
>> +/**
>> + * ice_fmsg_put_ptr - put hex value of pointer into fmsg
>> + *
>> + * @fmsg: devlink fmsg under construction
>> + * @name: name to pass
>> + * @ptr: 64 bit value to print as hex and put into fmsg
>> + */
>> +static void ice_fmsg_put_ptr(struct devlink_fmsg *fmsg, const char *name,
>> + void *ptr)
>> +{
>> + char buf[sizeof(ptr) * 3];
>> +
>> + sprintf(buf, "%p", ptr);
>> + devlink_fmsg_put(fmsg, name, buf);
>> +}
>
> ...
>
>> +static int ice_tx_hang_reporter_dump(struct devlink_health_reporter *reporter,
>> + struct devlink_fmsg *fmsg, void *priv_ctx,
>> + struct netlink_ext_ack *extack)
>> +{
>> + struct ice_tx_hang_event *event = priv_ctx;
>> +
>> + devlink_fmsg_obj_nest_start(fmsg);
>> + ICE_DEVLINK_FMSG_PUT_FIELD(fmsg, event, head);
>> + ICE_DEVLINK_FMSG_PUT_FIELD(fmsg, event, intr);
>> + ICE_DEVLINK_FMSG_PUT_FIELD(fmsg, event, vsi_num);
>> + ICE_DEVLINK_FMSG_PUT_FIELD(fmsg, event, queue);
>> + ICE_DEVLINK_FMSG_PUT_FIELD(fmsg, event, next_to_clean);
>> + ICE_DEVLINK_FMSG_PUT_FIELD(fmsg, event, next_to_use);
>> + devlink_fmsg_put(fmsg, "irq-mapping", event->tx_ring->q_vector->name);
>> + ice_fmsg_put_ptr(fmsg, "desc-ptr", event->tx_ring->desc);
>> + ice_fmsg_put_ptr(fmsg, "dma-ptr", (void *)event->tx_ring->dma);
>
> As reported by the kernel test robot, GCC 13 complains about this cast:
>
> .../devlink_health.c: In function 'ice_tx_hang_reporter_dump':
> .../devlink_health.c:76:43: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
> 76 | ice_fmsg_put_ptr(fmsg, "dma-ptr", (void *)event->tx_ring->dma);
> |
>
> Perhaps a good solution is to add a helper similar to ice_fmsg_put_ptr,
> but which takes a dma_buf_t rather than a void * as it's last argument.
instead of duplicating the function for just one call, I will simply
resolve the warning by yet another cast:
ice_fmsg_put_ptr(fmsg, "dma-ptr", (void *)(long)event->tx_ring->dma);
^^^^^^ // cast to long added
>
>> + devlink_fmsg_binary_pair_put(fmsg, "desc", event->tx_ring->desc,
>> + size_mul(event->tx_ring->count,
>> + sizeof(struct ice_tx_desc)));
Here I would drop size_mul(), as any wrong ::count value could easily
extent the dump past tx_ring memory, resulting in attempt at reading
past their page
And we are not really protecting against "too big" fmsg, as it is capped
anyway to 4-8K.
Perhaps fmsg-put also ::count to aid spotting such cases, but only if it
is not the default 256.
--
not a change request, just digression:
it would be nice for devlink_fmsg_binary_pair_put() to compress
"repeated same value", like hexdump(1) does.
>> + devlink_fmsg_obj_nest_end(fmsg);
>> +
>> + return 0;
>> +}
Powered by blists - more mailing lists