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

[Song, Yoong Siang]

On Tuesday, July 30, 2024 6:09 PM, Drewek, Wojciech <wojciech.drewek@xxxxxxxxx> wrote:

[...]

>> +	if (mrqe != IGC_MRQC_ENABLE_MQ && mrqe !=
>IGC_MRQC_ENABLE_RSS_MQ)
>> +		return false;
>
>just:
>return mrqe != IGC_MRQC_ENABLE_MQ && mrqe != IGC_MRQC_ENABLE_RSS_MQ
>

Hi Drewek Wojciech,
Thanks for your review comments.
I will improve the code accordingly in v2.
[...]

>> +	if (!igc_is_default_queue_supported(mrqc)) {
>> +		netdev_warn(adapter->netdev,
>> +			    "MRQ disabled: default RxQ is ignored.\n");
>
>Should we return an error here?

Yes, we should. I plan to refactor this patch to use
ethtool ntuple method, instead of sysfs.
I will consider your suggestion in v2.

>> +	}
>> +
>> +	return (mrqc & IGC_MRQC_DEFAULT_QUEUE_MASK) >>
>> +		IGC_MRQC_DEFAULT_QUEUE_SHIFT;
>
>use FIELD_GET here
>

Noted.
[...]

>> +	struct pci_dev *pdev = to_pci_dev(dev);
>> +	struct net_device *netdev = pci_get_drvdata(pdev);
>> +	struct igc_adapter *adapter = netdev_priv(netdev);
>> +	u32 default_rx_queue = igc_get_default_rx_queue(adapter);
>
>Use RCT rule
>

Noted.
[...]

>> +	struct pci_dev *pdev = to_pci_dev(dev);
>> +	struct net_device *netdev = pci_get_drvdata(pdev);
>> +	struct igc_adapter *adapter = netdev_priv(netdev);
>> +	u32 default_rx_queue;
>> +	int err;
>
>RCT
>

Noted.
[...]

>> +	err = sysfs_create_group(&adapter->pdev->dev.kobj, &attr_group);
>> +	if (err) {
>
>no need for brackets
>

Noted.
[...]

Thanks & Regards
Siang