Re: [PATCH net-next 08/12] amd-xgbe: Add ethtool show/set channels support

[Tom Lendacky <thomas.lendacky@amd.com>]

On 5/22/2018 12:35 AM, Jakub Kicinski wrote:
> On Mon, 21 May 2018 16:59:37 -0500, Tom Lendacky wrote:
>> +	rx = combined + channels->rx_count;
>> +	tx = combined + channels->tx_count;
>> +	netdev_notice(netdev, "final channel count assignment: combined=%u, rx-only=%u, tx-only=%u\n",
>> +		      min(rx, tx), rx - min(rx, tx), tx - min(rx, tx));
> 
> If user requests combined 0 rx 8 tx 8 they will end up with combined 8
> rx 0 tx 0.  Is that expected?

Yes, which is the reason that I issue the final channel count message. I
debated on how to do all this and looked at other drivers as well as the
ethtool man page and decided on this logic.

> 
> The man page clearly sayeth:
> 
>        -L --set-channels
>               Changes the numbers of channels of the specified network device.
> 
>            rx N   Changes the number of channels with only receive queues.
> 
>            tx N   Changes the number of channels with only transmit queues.
> 
>            other N
>                   Changes the number of channels used only for other  purposes
>                   e.g. link interrupts or SR-IOV co-ordination.
> 
>            combined N
>                   Changes the number of multi-purpose channels.
> 
> Note the use of word *only*.  There are drivers in tree which adhere to
> this interpretation and dutifully allocate separate IRQs if RX and TX
> channels are requested separately.

The amd-xgbe driver is not designed to allocate separate IRQs for Rx and
Tx.  In general, there is one IRQ for a channel of which Tx and Rx are
shared.  You can have more Tx channels than Rx channels or vice-versa, but
the min() of those numbers will be a combined Tx/Rx with the excess being
Tx or Rx only: e.g. combined 0 tx 8 rx 10 results in 8 combined channels
plus two Rx only channels.

I thought this was the most reasonable way to do this, please let me know
if there's a strong objection to this.

Thanks,
Tom

> 
> Which is not to claim that majority of existing drivers adhere to this
> wording :)
>