RE: [PATCH iwl-next,v1 2/3] igc: Add default Rx queue configuration via sysfs

From: Song, Yoong Siang
Date: Thu Aug 01 2024 - 03:20:43 EST


On Tuesday, July 30, 2024 5:59 PM, Kurt Kanzenbach <kurt@xxxxxxxxxxxxx> wrote:
>> --- a/drivers/net/ethernet/intel/igc/igc_regs.h
>> +++ b/drivers/net/ethernet/intel/igc/igc_regs.h
>> @@ -63,6 +63,12 @@
>> /* RSS registers */
>> #define IGC_MRQC 0x05818 /* Multiple Receive Control - RW */
>>
>> +/* MRQC register bit definitions */
>
>Nit: Now, the MRQC register definitions are scattered over two files:
>igc_regs.h and igc.h. igc.h has
>
>#define IGC_MRQC_ENABLE_RSS_MQ 0x00000002
>#define IGC_MRQC_RSS_FIELD_IPV4_UDP 0x00400000
>#define IGC_MRQC_RSS_FIELD_IPV6_UDP 0x00800000
>
>Maybe combine them into a single location?
>

Hi Kurt Kanzenbach,

Thanks for your review comment.
Sure, I will try to combine them into single location.

>> +#define IGC_MRQC_ENABLE_MQ 0x00000000
>> +#define IGC_MRQC_ENABLE_MASK GENMASK(2, 0)
>> +#define IGC_MRQC_DEFAULT_QUEUE_MASK GENMASK(5, 3)
>> +#define IGC_MRQC_DEFAULT_QUEUE_SHIFT 3
>
>Nit: FIELD_GET() and FIELD_PREP() can help to get rid of the manual
>shifting. See below.
>

Noted.
[...]

>> + return (mrqc & IGC_MRQC_DEFAULT_QUEUE_MASK) >>
>> + IGC_MRQC_DEFAULT_QUEUE_SHIFT;
>
>Nit: return FIELD_GET(IGC_MRQC_DEFAULT_QUEUE_MASK, mrqc);
>

Noted.
[...]

>> + mrqc &= ~IGC_MRQC_DEFAULT_QUEUE_MASK;
>> + mrqc |= queue << IGC_MRQC_DEFAULT_QUEUE_SHIFT;
>
>Nit: mrqc |= FIELD_PREP(IGC_MRQC_DEFAULT_QUEUE_MASK, queue);
>

Noted.
[...]

>Thanks,
>Kurt

Thanks & Regards
Siang