| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Fri, 17 Mar 2017 11:18:04 +0000
From: Joao Pinto <Joao.Pinto@...opsys.com>
To: Florian Fainelli <f.fainelli@...il.com>,
Joao Pinto <Joao.Pinto@...opsys.com>, <davem@...emloft.net>
CC: <peppe.cavallaro@...com>, <alexandre.torgue@...com>,
<netdev@...r.kernel.org>
Subject: Re: [PATCH net-next 3/3] net: stmmac: RX queue routing configuration
Às 5:12 PM de 3/16/2017, Florian Fainelli escreveu:
> On 03/16/2017 03:56 AM, Joao Pinto wrote:
>> This patch adds the configuration of RX queues' routing.
>>
>> Signed-off-by: Joao Pinto <jpinto@...opsys.com>
>> ---
>> Documentation/devicetree/bindings/net/stmmac.txt | 19 ++++++-
>> drivers/net/ethernet/stmicro/stmmac/common.h | 9 ++++
>> drivers/net/ethernet/stmicro/stmmac/dwmac4.h | 18 +++++++
>> drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c | 63 ++++++++++++++++++++++
>> drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 25 +++++++++
>> drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c | 8 +++
>> .../net/ethernet/stmicro/stmmac/stmmac_platform.c | 49 +++++++++++++++++
>> include/linux/stmmac.h | 12 +++++
>> 8 files changed, 202 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/devicetree/bindings/net/stmmac.txt b/Documentation/devicetree/bindings/net/stmmac.txt
>> index b9a9ae9..9c9e774 100644
>> --- a/Documentation/devicetree/bindings/net/stmmac.txt
>> +++ b/Documentation/devicetree/bindings/net/stmmac.txt
>> @@ -84,6 +84,16 @@ Optional properties:
>> - snps,avb-algorithm: Queue to be enabled as AVB
>> - snps,map-to-dma-channel: Channel to map
>> - snps,priority: RX queue priority
>> + - Specifiy RX Queue for processing certain types of packets:
>> + - snps,avcp-to-queue: AV Untagged Control packets
>> + - snps,ptp-to-queue: PTP Packets
>> + - snps,dcbcp-to-queue: DCB Control Packets
>> + - snps,up-to-queue: Untagged Packets
>> + - snps,multi-broad-cast-queue: Multicast & Broadcast Packets
>> + - snps,tpqc-queue: Tagged PTPoE Packets Queuing Control. Values:
>> + 0x0 - Priority based (RX queues non-AV)
>> + 0x1 - Routes to queue indicated in "snps,ptp-to-queue"
>> + 0x2 - Priority based (RX queues AV enabled)
>> - Multiple TX Queues parameters: below the list of all the parameters to
>> configure the multiple TX queues:
>> - snps,tx-queues-to-use: number of TX queues to be used in the driver
>
> You are allowing a policy to be defined with Device Tree here which is
> usually frowned upon. Can you look into existing facilities offered by
> tc offloads, e.g: mqprio etc. to allow users to dynamically configure
> the queue mappings at runtime?
We are planning also to have these configurable in runtime by a standard tool
like ethtool, but I think we should have the DT possibility also. Makes sense
for you?
>
>> @@ -112,8 +122,15 @@ Examples:
>> };
>>
>> mtl_rx_setup: rx-queues-config {
>> - snps,rx-queues-to-use = <1>;
>> + snps,rx-queues-to-use = <0>;
>> snps,rx-sched-sp;
>> + snps,avcp-to-queue = <0>;
>> + snps,ptp-to-queue = <0>;
>> + snps,dcbcp-to-queue = <0>;
>> + snps,up-to-queue = <0>;
>> + snps,multi-broad-cast-queue = <0>;
>> + snps,tpqc-queue = <0>;
>> +
>> queue0 {
>> snps,dcb-algorithm;
>> snps,map-to-dma-channel = <0x0>;
>> diff --git a/drivers/net/ethernet/stmicro/stmmac/common.h b/drivers/net/ethernet/stmicro/stmmac/common.h
>> index e0b31e7..e08c4c4 100644
>> --- a/drivers/net/ethernet/stmicro/stmmac/common.h
>> +++ b/drivers/net/ethernet/stmicro/stmmac/common.h
>> @@ -473,6 +473,15 @@ struct stmmac_ops {
>> void (*rx_queue_prio)(struct mac_device_info *hw, u32 prio, u32 queue);
>> /* TX Queues Priority */
>> void (*tx_queue_prio)(struct mac_device_info *hw, u32 prio, u32 queue);
>> + /* RX Queues Routing */
>> + void (*rx_queue_routing)(struct mac_device_info *hw,
>> + bool route_avcp_enable, u8 route_avcp_queue,
>> + bool route_ptp_enable, u8 route_ptp_queue,
>> + bool route_dcbcp_enable, u8 route_dcbcp_queue,
>> + bool route_up_enable, u8 route_up_queue,
>> + bool route_multi_broad_enable,
>> + u8 route_multi_broad_queue,
>> + bool route_tpqc_enable, u8 route_tpqc_queue);
>> /* Program RX Algorithms */
>> void (*prog_mtl_rx_algorithms)(struct mac_device_info *hw, u32 rx_alg);
>> /* Program TX Algorithms */
>> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac4.h b/drivers/net/ethernet/stmicro/stmmac/dwmac4.h
>> index a6c382d..93cc3f6 100644
>> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac4.h
>> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac4.h
>> @@ -44,6 +44,24 @@
>> #define GMAC_ADDR_HIGH(reg) (0x300 + reg * 8)
>> #define GMAC_ADDR_LOW(reg) (0x304 + reg * 8)
>>
>> +/* RX Queues Routing */
>> +#define GMAC_RXQCTRL_AVCPQ_MASK GENMASK(2, 0)
>> +#define GMAC_RXQCTRL_AVCPQ_SHIFT 0
>> +#define GMAC_RXQCTRL_PTPQ_MASK GENMASK(6, 4)
>> +#define GMAC_RXQCTRL_PTPQ_SHIFT 4
>> +#define GMAC_RXQCTRL_DCBCPQ_MASK GENMASK(10, 8)
>> +#define GMAC_RXQCTRL_DCBCPQ_SHIFT 8
>> +#define GMAC_RXQCTRL_UPQ_MASK GENMASK(14, 12)
>> +#define GMAC_RXQCTRL_UPQ_SHIFT 12
>> +#define GMAC_RXQCTRL_MCBCQ_MASK GENMASK(18, 16)
>> +#define GMAC_RXQCTRL_MCBCQ_SHIFT 16
>> +#define GMAC_RXQCTRL_MCBCQEN BIT(20)
>> +#define GMAC_RXQCTRL_MCBCQEN_SHIFT 20
>> +#define GMAC_RXQCTRL_TACPQE BIT(21)
>> +#define GMAC_RXQCTRL_TACPQE_SHIFT 21
>> +#define GMAC_RXQCTRL_TPQC_MASK GENMASK(24, 22)
>> +#define GMAC_RXQCTRL_TPQC_SHIFT 22
>> +
>> /* MAC Packet Filtering */
>> #define GMAC_PACKET_FILTER_PR BIT(0)
>> #define GMAC_PACKET_FILTER_HMC BIT(2)
>> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c b/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c
>> index 342f62a..8fc1f56 100644
>> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c
>> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c
>> @@ -109,6 +109,68 @@ static void dwmac4_tx_queue_priority(struct mac_device_info *hw,
>> writel(value, ioaddr + base_register);
>> }
>>
>> +static void dwmac4_tx_queue_routing(struct mac_device_info *hw,
>> + bool route_avcp_enable,
>> + u8 route_avcp_queue,
>> + bool route_ptp_enable,
>> + u8 route_ptp_queue,
>> + bool route_dcbcp_enable,
>> + u8 route_dcbcp_queue,
>> + bool route_up_enable,
>> + u8 route_up_queue,
>> + bool route_multi_broad_enable,
>> + u8 route_multi_broad_queue,
>> + bool route_tpqc_enable,
>> + u8 route_tpqc_queue)
>> +{
>
> This is not looking good at all 13 parameters passed here... thankfully
> this is not called in a hot path.
>
> So here is what I would suggest:
>
> - put the bool, u8, and masks, shifts, offsets parameters in a
> structure, define an array of structures holding these variables in your
> driver's private context, index them with an enumeration for each queue
>
> - pass a reference to this structure here, and iterate over the array of
> structures and do the programming based on what this structure contains
> for MASK, SHIFT, base register etc...
Good suggestion, I will follow that, thanks.
>
>> + void __iomem *ioaddr = hw->pcsr;
>> + u32 value;
>> +
>> + value = readl(ioaddr + GMAC_RXQ_CTRL1);
>> +
>
>> static int quark_default_data(struct plat_stmmacenet_data *plat,
>> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
>> index 77b0468..5e82379 100644
>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
>> @@ -165,6 +165,55 @@ static void stmmac_mtl_setup(struct platform_device *pdev,
>> else
>> plat->rx_sched_algorithm = MTL_RX_ALGORITHM_SP;
>>
>> + /* RX routing configuration */
>> + if (of_property_read_u8(rx_node, "snps,avcp-to-queue",
>> + &plat->route_avcp_queue)) {
>> + plat->route_avcp_queue = 0;
>> + plat->route_avcp_enable = false;
>> + } else {
>> + plat->route_avcp_enable = true;
>> + }
>
> This is very redundant too, see the suggestion above, define an array of
> property names and look them up in a loop using the queue enumeration.
>
>> +
>> + if (of_property_read_u8(rx_node, "snps,ptp-to-queue",
>> + &plat->route_ptp_queue)) {
>> + plat->route_ptp_queue = 0;
>> + plat->route_ptp_enable = false;
>> + } else {
>> + plat->route_ptp_enable = true;
>> + }
>> +
>> + if (of_property_read_u8(rx_node, "snps,dcbcp-to-queue",
>> + &plat->route_dcbcp_queue)) {
>> + plat->route_dcbcp_queue = 0;
>> + plat->route_dcbcp_enable = false;
>> + } else {
>> + plat->route_dcbcp_enable = true;
>> + }
>> +
>> + if (of_property_read_u8(rx_node, "snps,up-to-queue",
>> + &plat->route_up_queue)) {
>> + plat->route_up_queue = 0;
>> + plat->route_up_enable = false;
>> + } else {
>> + plat->route_up_enable = true;
>> + }
>> +
>> + if (of_property_read_u8(rx_node, "snps,multi-broad-cast-queue",
>> + &plat->route_multi_broad_queue)) {
>> + plat->route_multi_broad_queue = 0;
>> + plat->route_multi_broad_enable = false;
>> + } else {
>> + plat->route_multi_broad_enable = true;
>> + }
>> +
>> + if (of_property_read_u8(rx_node, "snps,tpqc-queue",
>> + &plat->route_tpqc_queue)) {
>> + plat->route_tpqc_queue = 0;
>> + plat->route_tpqc_enable = false;
>> + } else {
>> + plat->route_tpqc_enable = true;
>> + }
>> +
>> /* Processing individual RX queue config */
>> for_each_child_of_node(rx_node, q_node) {
>> if (queue >= plat->rx_queues_to_use)
>> diff --git a/include/linux/stmmac.h b/include/linux/stmmac.h
>> index b7d5e7a..8f5ebf5 100644
>> --- a/include/linux/stmmac.h
>> +++ b/include/linux/stmmac.h
>> @@ -171,6 +171,18 @@ struct plat_stmmacenet_data {
>> u8 tx_queues_to_use;
>> u8 rx_sched_algorithm;
>> u8 tx_sched_algorithm;
>> + bool route_avcp_enable;
>> + u8 route_avcp_queue;
>> + bool route_ptp_enable;
>> + u8 route_ptp_queue;
>> + bool route_dcbcp_enable;
>> + u8 route_dcbcp_queue;
>> + bool route_up_enable;
>> + u8 route_up_queue;
>> + bool route_multi_broad_enable;
>> + u8 route_multi_broad_queue;
>> + bool route_tpqc_enable;
>> + u8 route_tpqc_queue;
>
> Same here, this absolutely does not scale.
>
>> struct stmmac_rxq_cfg rx_queues_cfg[MTL_MAX_RX_QUEUES];
>> struct stmmac_txq_cfg tx_queues_cfg[MTL_MAX_TX_QUEUES];
>> void (*fix_mac_speed)(void *priv, unsigned int speed);
>>
>
>
Powered by blists - more mailing lists