Vladimir Oltean <vladimir.olt...@nxp.com> writes:
> On Fri, Jan 22, 2021 at 02:44:51PM -0800, Vinicius Costa Gomes wrote:
>> The tc subsystem sets which queues are marked as preemptible, it's the
>> role of ethtool to control more hardware specific parameters. These
>> parameters include:
>>
>> - enabling the frame preemption hardware: As enabling frame
>> preemption may have other requirements before it can be enabled, it's
>> exposed via the ethtool API;
>>
>> - mininum fragment size multiplier: expressed in usually in the form
>> of (1 + N)*64, this number indicates what's the size of the minimum
>> fragment that can be preempted.
>
> And not one word has been said about the patch...
If I am undertanding this right. Will fix the commit message.
>
>>
>> Signed-off-by: Vinicius Costa Gomes <vinicius.go...@intel.com>
>> ---
>> drivers/net/ethernet/intel/igc/igc.h | 12 +++++
>> drivers/net/ethernet/intel/igc/igc_defines.h | 4 ++
>> drivers/net/ethernet/intel/igc/igc_ethtool.c | 53 ++++++++++++++++++++
>> drivers/net/ethernet/intel/igc/igc_tsn.c | 25 +++++++--
>> 4 files changed, 91 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/igc/igc.h
>> b/drivers/net/ethernet/intel/igc/igc.h
>> index 35baae900c1f..1067c46e0bc2 100644
>> --- a/drivers/net/ethernet/intel/igc/igc.h
>> +++ b/drivers/net/ethernet/intel/igc/igc.h
>> @@ -87,6 +87,7 @@ struct igc_ring {
>> u8 queue_index; /* logical index of the ring*/
>> u8 reg_idx; /* physical index of the ring */
>> bool launchtime_enable; /* true if LaunchTime is enabled */
>> + bool preemptible; /* true if not express */
>
> Mixing tabs and spaces?
Ugh. Will fix. Thanks.
>
>> +static int igc_ethtool_set_preempt(struct net_device *netdev,
>> + struct ethtool_fp *fpcmd,
>> + struct netlink_ext_ack *extack)
>> +{
>> + struct igc_adapter *adapter = netdev_priv(netdev);
>> + int i;
>> +
>> + if (fpcmd->add_frag_size < 68 || fpcmd->add_frag_size > 260) {
>> + NL_SET_ERR_MSG_MOD(extack, "Invalid value for add-frag-size");
>> + return -EINVAL;
>> + }
>
> This check should belong in ethtool, since there's nothing unusual about
> this supported range.
>
> Also, I believe that Jakub requested the min-frag-size to be passed as
> 0, 1, 2, 3 as the standard specifies it, and not its multiplied
> version?
Later, Michal Kubechek suggested using the multiplied value, to be
future proof and less dependent on some specific standard version.
>
>> +
>> + adapter->frame_preemption_active = fpcmd->enabled;
>> + adapter->add_frag_size = fpcmd->add_frag_size;
>> +
>> + if (!adapter->frame_preemption_active)
>> + goto done;
>> +
>> + /* Enabling frame preemption requires TSN mode to be enabled,
>> + * which requires a schedule to be active. So, if there isn't
>> + * a schedule already configured, configure a simple one, with
>> + * all queues open, with 1ms cycle time.
>> + */
>> + if (adapter->base_time)
>> + goto done;
>
> Unless I'm missing something, you are interpreting an adapter->base_time
> value of zero as "no Qbv schedule on port", as if it was invalid to have
> a base-time of zero, which it isn't.
This HW has specific limitations, it doesn't allow a base_time in the
past. So a base_time of zero can be used to signify "No Qbv".
>
>> @@ -115,6 +130,9 @@ static int igc_tsn_enable_offload(struct igc_adapter
>> *adapter)
>> if (ring->launchtime_enable)
>> txqctl |= IGC_TXQCTL_QUEUE_MODE_LAUNCHT;
>>
>> + if (ring->preemptible)
>> + txqctl |= IGC_TXQCTL_PREEMPTABLE;
>
> I think this is the only place in the series where you use PREEMPTABLE
> instead of PREEMPTIBLE.
Yeah, on the datasheet it's written PREEMPTABLE, I chose to use this
spelling to make it easier to search for this bit in the datasheet.
>
>> +
>> wr32(IGC_TXQCTL(i), txqctl);
>> }
>
> Out of curiosity, where is the ring to traffic class mapping configured
> in the igc driver? I suppose that you have more rings than traffic classes.
The driver follows the default behaviour, that netdev->queue[0] maps to
ring[0], queue[1] to ring[1], and so on. And by default ring[0] has
higher priority than ring[1], ring[1] higher than ring[2], and so on.
The HW only has 4 rings/queues.
Cheers,
--
Vinicius
