* Re: [v5, PATCH 1/2] net:stmmac: dwmac-mediatek: add support for mt2712
From: biao huang @ 2018-11-23 9:09 UTC (permalink / raw)
To: Sean Wang
Cc: davem, robh+dt, mark.rutland, devicetree, nelson.chang,
Andrew Lunn, netdev, Liguo Zhang, linux-kernel, Matthias Brugger,
joabreu, linux-mediatek, honghui.zhang, linux-arm-kernel
In-Reply-To: <CAGp9LzrdwXqQ+aHtCoKcxKx28xR21hKnpc+ruW4Gt7TyQLffsA@mail.gmail.com>
Dear Sean,
Thanks for your comments.
If any misunderstanding, please correct me.
On Thu, 2018-11-22 at 23:04 -0800, Sean Wang wrote:
> < ... >
>
> > > > + /* select phy interface in top control domain */
> > > > + switch (plat->phy_mode) {
> > > > + case PHY_INTERFACE_MODE_MII:
> > > > + intf_val |= PHY_INTF_MII_GMII;
> > > > + break;
> > > > + case PHY_INTERFACE_MODE_RMII:
> > > > + intf_val |= PHY_INTF_RMII;
> > > > + intf_val |= rmii_rxc;
> > > how about putting into one line such as intf_val |= (PHY_INTF_RMII | rmii_rxc) ?
> > >
> > ok, will change in next version.
> > > > + break;
> > > > + case PHY_INTERFACE_MODE_RGMII:
> > > > + case PHY_INTERFACE_MODE_RGMII_TXID:
> > > > + case PHY_INTERFACE_MODE_RGMII_RXID:
> > > > + case PHY_INTERFACE_MODE_RGMII_ID:
> > > > + intf_val |= PHY_INTF_RGMII;
> > > > + break;
> > > > + default:
> > > > + dev_err(plat->dev, "phy interface not supported\n");
> > > > + return -EINVAL;
> > > > + }
> > > > +
> > > > + regmap_write(plat->peri_regmap, PERI_ETH_PHY_INTF_SEL, intf_val);
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static int mt2712_set_delay(struct mediatek_dwmac_plat_data *plat)
> > > > +{
> > > > + struct mac_delay_struct *mac_delay = &plat->mac_delay;
> > > > + u32 delay_val = 0;
> > > > + u32 fine_val = 0;
> > > The same type declaration can be put into the one line
> > >
> > Got it.
> > > > +
> > > > + switch (plat->phy_mode) {
> > >
> > > There exists some room for code optimization in the switch statement
> > > such as PHY_INTERFACE_MODE_MII and PHY_INTERFACE_MODE_RGMII both are
> > > almost the same and even the configuration for the other PHY modes can
> > > reuse their partial setup. It appears to be better using a common way
> > > to set up various PHY modes.
> > >
> > I'll try define a function to handle it.
> > And how about like this:
> > static u32 delay_setting(u32 delay, bool inv, u32 en_bit, u32
> > clk_shift, u32 clk_mask, u32 inv_bit) {
> > u32 value = 0
> >
> > value |= delay ? en_bit : 0;
> > value |= (delay << clk_shift) & clk_mask;
> > value |= inv ? inv_bit : 0;
> > }
> >
> > case PHY_INTERFACE_MODE_MII:
> > delay_value |= delay_setting(mac_delay->tx_delay,
> > mac_delay->tx_inv,
> > PHY_DLY_TXC_ENABLE,
> > PHY_DLY_TXC_SHIFT,
> > PHY_DLY_TXC_STAGES,
> > PHY_DLY_TXC_INV);
>
> We can reuse FIELD_PREP defined in include/linux/bitfield.h to make up
> of the value instead of creating your own function delay_setting here,
> and also PHY_DLY_TXC_SHIFT macro can be trimmed while you're using
> FIED_PREP
>
ok, so it should like this:
delay_value |= FIELD_PREP(PHY_DLY_TXC_ENABLE, !!mac_delay->delay);
delay_value |= FIELD_PREP(PHY_DLY_TXC_STAGES, mac_delay->delay);
delay_value |= FIELD_PREP(PHY_DLY_TXC_INV, mac_delay->inv);
> > delay_value |= delay_setting(mac_delay->rx_delay,
> > mac_delay->rx_inv,
> > PHY_DLY_RXC_ENABLE,
> > PHY_DLY_RXC_SHIFT,
> > PHY_DLY_RXC_STAGES,
> > PHY_DLY_RXC_INV);
> >
> > case PHY_INTERFACE_MODE_RMII:
> > if (plat->rmii_rxc) {
> > delay_value |= delay_setting(mac_delay->rx_delay,
> > mac_delay->rx_inv,
> > PHY_DLY_RXC_ENABLE,
> > PHY_DLY_RXC_SHIFT,
> > PHY_DLY_RXC_STAGES,
> > PHY_DLY_RXC_INV);
> > fine_val |= mac_delay->tx_inv ?
> > ETH_RMII_DLY_TX_INV : 0;
> > } else {
> > delay_value |= delay_setting(mac_delay->rx_delay,
> > mac_delay->rx_inv,
>
> shoudn't the parametors be mac_delay->tx_delay and mac_delay->tx_inv?
>
in this case, the rmii reference clk from external phy is connected to
TXC pin, and property "rmii_rxc" will handle which pin the reference clk
is connected to. after that, the reference clk is only a received clk
from outside, so use rx_delay/rx_inv may be much better.
> > PHY_DLY_TXC_ENABLE,
> > PHY_DLY_TXC_SHIFT,
> > PHY_DLY_TXC_STAGES,
> > PHY_DLY_TXC_INV);
> > fine_val |= mac_delay->tx_inv ?
> > ETH_RMII_DLY_TX_INV : 0;
>
> if (plat->tx_inv)
> fine_val = ETH_RMII_DLY_TX_INV;
> the default fine_val is zero so zero assignement can be trimmed when
> !plat-> tx_inv
>
> > }
> > case PHY_INTERFACE_MODE_RGMII:
> > fine_val = plat->fine_tune ?
> > (ETH_FINE_DLY_GTXC | ETH_FINE_DLY_RXC) : 0;
>
> if (plat->fine_tune)
> fine_val = ETH_FINE_DLY_GTXC | ETH_FINE_DLY_RXC;
> the default fine_val is zero so zero assignement can be trimmed when
> !plat->fine_tune
>
ok, I'll not initialize fine_val.
> > delay_value |= delay_setting(mac_delay->tx_delay,
> > mac_delay->tx_inv,
> > PHY_DLY_GTXC_ENABLE,
> > PHY_DLY_GTXC_SHIFT,
> > PHY_DLY_GTXC_STAGES,
> > PHY_DLY_GTXC_INV);
> > delay_value |= delay_setting(mac_delay->rx_delay,
> > mac_delay->rx_inv,
> > PHY_DLY_RXC_ENABLE,
> > PHY_DLY_RXC_SHIFT,
> > PHY_DLY_RXC_STAGES,
> > PHY_DLY_RXC_INV);
> > case PHY_INTERFACE_MODE_RGMII_TXID:
> > fine_val = plat->fine_tune ? ETH_FINE_DLY_RXC : 0;
> > delay_value |= delay_setting(mac_delay->rx_delay,
> > mac_delay->rx_inv,
> > PHY_DLY_RXC_ENABLE,
> > PHY_DLY_RXC_SHIFT,
> > PHY_DLY_RXC_STAGES,
> > PHY_DLY_RXC_INV);
> > case PHY_INTERFACE_MODE_RGMII_RXID:
> > fine_val = plat->fine_tune ? ETH_FINE_DLY_GTXC : 0;
> > delay_value |= delay_setting(mac_delay->tx_delay,
> > mac_delay->tx_inv,
> > PHY_DLY_GTXC_ENABLE,
> > PHY_DLY_GTXC_SHIFT,
> > PHY_DLY_GTXC_STAGES,
> > PHY_DLY_GTXC_INV);
> >
>
> phy_mode is used to indicate what phy mode would be tweaked when mac
> is connected to the phy so I thought mac delay can be independent from
> phy internal delay that means PHY_INTERFACE_MODE_RGMII_RXID and
> PHY_INTERFACE_MODE_RGMII_TXID can apply the same setting as
> PHY_INTERFACE_MODE_RGMII does.
>
no, when phy adjust the tx delay, mac should not adjust tx delay at the
same time. ex: phy will delay 2ns, and mac delay 2ns, the total delay
will be 2+2=4ns.the whole delay will not meet the rgmii requirement.
And PHY_INTERFACE_MODE_RGMII_TXID/RXID/ID is clarified clearly in
phy.txt, I think it should be ok here.
More, I'll add comments here to avoid confusion.
> > > > + case PHY_INTERFACE_MODE_MII:
> > > > + delay_val |= mac_delay->tx_delay ? PHY_DLY_TXC_ENABLE : 0;
> > > > + delay_val |= (mac_delay->tx_delay << PHY_DLY_TXC_SHIFT) &
> > > > + PHY_DLY_TXC_STAGES;
> > > > + delay_val |= mac_delay->tx_inv ? PHY_DLY_TXC_INV : 0;
> > > > + delay_val |= mac_delay->rx_delay ? PHY_DLY_RXC_ENABLE : 0;
> > > > + delay_val |= (mac_delay->rx_delay << PHY_DLY_RXC_SHIFT) &
> > > > + PHY_DLY_RXC_STAGES;
> > > > + delay_val |= mac_delay->rx_inv ? PHY_DLY_RXC_INV : 0;
> > > > + break;
> > > > + case PHY_INTERFACE_MODE_RMII:
> > > > + if (plat->rmii_rxc) {
> > > > + delay_val |= mac_delay->rx_delay ?
> > > > + PHY_DLY_RXC_ENABLE : 0;
> > > > + delay_val |= (mac_delay->rx_delay <<
> > > > + PHY_DLY_RXC_SHIFT) & PHY_DLY_RXC_STAGES;
> > > > + delay_val |= mac_delay->rx_inv ? PHY_DLY_RXC_INV : 0;
> > > > + fine_val |= mac_delay->tx_inv ?
> > > > + ETH_RMII_DLY_TX_INV : 0;
> > > why is fine_val got from tx_inv?
> > >
> > becase the tx inv will inverse the tx clock inside mac relative to
> > reference clock from external phy, and this bit is located in the same
> > register with fine-tune.
> > maybe I should rename fine_val to avoid misunderstanding.
>
> If you add more comments to say that, fine_val remains would be okay
>
OK, I'll add comments.
> > > > + } else {
> > > > + delay_val |= mac_delay->rx_delay ?
> > > > + PHY_DLY_TXC_ENABLE : 0;
> > > > + delay_val |= (mac_delay->rx_delay <<
> > > > + PHY_DLY_TXC_SHIFT) & PHY_DLY_TXC_STAGES;
> > > > + delay_val |= mac_delay->rx_inv ? PHY_DLY_TXC_INV : 0;
> > > > + fine_val |= mac_delay->tx_inv ?
> > > > + ETH_RMII_DLY_TX_INV : 0;
> > > ditto, why is fine_val got from tx_inv?
> > >
> > same as above. ETH_RMII_DLY_TX_INV is only for RMII, and located in the
> > same register with fine-tune.
>
> adding a fewer comments helps to avoid some confusion
>
OK.
> > > > + }
> > > > + break;
> > > > + case PHY_INTERFACE_MODE_RGMII:
> > > > + fine_val = plat->fine_tune ?
> > > > + (ETH_FINE_DLY_GTXC | ETH_FINE_DLY_RXC) : 0;
> > > > + delay_val |= mac_delay->tx_delay ? PHY_DLY_GTXC_ENABLE : 0;
> > > > + delay_val |= mac_delay->tx_delay & PHY_DLY_GTXC_STAGES;
> > > > + delay_val |= mac_delay->tx_inv ? PHY_DLY_GTXC_INV : 0;
> > > > + delay_val |= mac_delay->rx_delay ? PHY_DLY_RXC_ENABLE : 0;
> > > > + delay_val |= (mac_delay->rx_delay << PHY_DLY_RXC_SHIFT) &
> > > > + PHY_DLY_RXC_STAGES;
> > > > + delay_val |= mac_delay->rx_inv ? PHY_DLY_RXC_INV : 0;
> > > > + break;
> > > > + case PHY_INTERFACE_MODE_RGMII_TXID:
> > > > + fine_val = plat->fine_tune ? ETH_FINE_DLY_RXC : 0;
> > > > + delay_val |= mac_delay->rx_delay ? PHY_DLY_RXC_ENABLE : 0;
> > > > + delay_val |= (mac_delay->rx_delay << PHY_DLY_RXC_SHIFT) &
> > > > + PHY_DLY_RXC_STAGES;
> > > > + delay_val |= mac_delay->rx_inv ? PHY_DLY_RXC_INV : 0;
> > > why is PHY_INTERFACE_MODE_RGMII_TXID applied with *_RXC_* register
> > > bits, not with *_TXC_* bits? I'm a little confused about what path the
> > > register PHY_DLY_RXC_* cause the effects to? MAC to PHY or PHY to MAC?
> > >
> > The PHY_INTERFACE_MODE_RGMII_TXID is defined in
> > Documentation/networking/phy.txt
> > means phy will handle delay in tx path, so mac need handle delay in rx
> > path here.
>
> See the above explains: phy_mode is used to indicate what phy mode
> would be tweaked when mac is connected to the phy so I thought mac
> delay can be independent of phy internal delay that means
> PHY_INTERFACE_MODE_RGMII_RXID and PHY_INTERFACE_MODE_RGMII_TXID can
> apply the same setting as PHY_INTERFACE_MODE_RGMII does.
>
Maybe more comments will avoid confusion.
> > > > + break;
> > > > + case PHY_INTERFACE_MODE_RGMII_RXID:
> > > > + fine_val = plat->fine_tune ? ETH_FINE_DLY_GTXC : 0;
> > > > + delay_val |= mac_delay->tx_delay ? PHY_DLY_GTXC_ENABLE : 0;
> > > > + delay_val |= mac_delay->tx_delay & PHY_DLY_GTXC_STAGES;
> > > > + delay_val |= mac_delay->tx_inv ? PHY_DLY_GTXC_INV : 0;
> > > ditto, as the above quetion
> > >
> > Similar answer as above.
> > > > + break;
> > > > + case PHY_INTERFACE_MODE_RGMII_ID:
> > > > + break;
> > > > + default:
> > > > + dev_err(plat->dev, "phy interface not supported\n");
> > > > + return -EINVAL;
> > > > + }
> > > > + regmap_write(plat->peri_regmap, PERI_ETH_PHY_DLY, delay_val);
> > > > + regmap_write(plat->peri_regmap, PERI_ETH_DLY_FINE, fine_val);
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static const struct mediatek_dwmac_variant mt2712_gmac_variant = {
> > > > + .dwmac_set_phy_interface = mt2712_set_interface,
> > > > + .dwmac_set_delay = mt2712_set_delay,
> > > > + .clk_list = mt2712_dwmac_clk_l,
> > > > + .num_clks = ARRAY_SIZE(mt2712_dwmac_clk_l),
> > > > + .dma_bit_mask = 33,
> > > > + .rx_delay_max = 32,
> > > > + .tx_delay_max = 32,
> > > > +};
> > > > +
> > > > +static int mediatek_dwmac_config_dt(struct mediatek_dwmac_plat_data *plat)
> > > > +{
> > > > + u32 tx_delay, rx_delay;
> > > > +
> > > > + plat->peri_regmap = syscon_regmap_lookup_by_phandle(plat->np, "mediatek,pericfg");
>
> you're also missing the property definition in dt-binding.
>
thanks for remind. I'll add in next patch.
> > > > + if (IS_ERR(plat->peri_regmap)) {
> > > > + dev_err(plat->dev, "Failed to get pericfg syscon\n");
> > > > + return PTR_ERR(plat->peri_regmap);
> > > > + }
> > > > +
> > > > + plat->phy_mode = of_get_phy_mode(plat->np);
> > > > + if (plat->phy_mode < 0) {
> > > > + dev_err(plat->dev, "not find phy-mode\n");
> > > > + return -EINVAL;
> > > > + }
> > > > +
> > > > + if (!of_property_read_u32(plat->np, "mediatek,tx-delay", &tx_delay)) {
> > > > + if (tx_delay < plat->variant->tx_delay_max) {
> > > > + plat->mac_delay.tx_delay = tx_delay;
> > > > + } else {
> > > > + dev_err(plat->dev, "Invalid TX clock delay: %d\n", tx_delay);
> > > > + return -EINVAL;
> > > > + }
> > > > + }
> > > > +
> > > > + if (!of_property_read_u32(plat->np, "mediatek,rx-delay", &rx_delay)) {
> > > > + if (rx_delay < plat->variant->rx_delay_max) {
> > > > + plat->mac_delay.rx_delay = rx_delay;
> > > > + } else {
> > > > + dev_err(plat->dev, "Invalid RX clock delay: %d\n", rx_delay);
> > > > + return -EINVAL;
> > > > + }
> > > > + }
> > > > +
> > > > + plat->mac_delay.tx_inv = of_property_read_bool(plat->np, "mediatek,txc-inverse");
> > > > + plat->mac_delay.rx_inv = of_property_read_bool(plat->np, "mediatek,rxc-inverse");
> > > > + plat->fine_tune = of_property_read_bool(plat->np, "mediatek,fine-tune");
> > > > + plat->rmii_rxc = of_property_read_bool(plat->np, "mediatek,rmii-rxc");
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static int mediatek_dwmac_clk_init(struct mediatek_dwmac_plat_data *plat)
> > > > +{
> > > > + const struct mediatek_dwmac_variant *variant = plat->variant;
> > > > + int num = variant->num_clks;
> > > > + int i;
> > > put into the same line seems good
> > >
> > ok
> > > > +
> > > > + plat->clks = devm_kcalloc(plat->dev, num, sizeof(*plat->clks), GFP_KERNEL);
> > > > + if (!plat->clks)
> > > > + return -ENOMEM;
> > > > +
> > > > + for (i = 0; i < num; i++)
> > > > + plat->clks[i].id = variant->clk_list[i];
> > > > +
> > > > + return devm_clk_bulk_get(plat->dev, num, plat->clks);
> > > > +}
> > > > +
> > > > +static int mediatek_dwmac_init(struct platform_device *pdev, void *priv)
> > > > +{
> > > > + struct mediatek_dwmac_plat_data *plat = priv;
> > > > + const struct mediatek_dwmac_variant *variant = plat->variant;
> > > > + int ret = 0;
> > > zero initialized seems unnecessary
> > >
> > ok, will not initialized here
> > > > +
> > > > + ret = dma_set_mask_and_coherent(plat->dev, DMA_BIT_MASK(variant->dma_bit_mask));
> > > > + if (ret) {
> > > > + dev_err(plat->dev, "No suitable DMA available, err = %d\n", ret);
> > > > + return ret;
> > > > + }
> > > > +
> > > > + ret = variant->dwmac_set_phy_interface(plat);
> > > > + if (ret) {
> > > > + dev_err(plat->dev, "failed to set phy interface, err = %d\n", ret);
> > > > + return ret;
> > > > + }
> > > > +
> > > > + ret = variant->dwmac_set_delay(plat);
> > > > + if (ret) {
> > > > + dev_err(plat->dev, "failed to set delay value, err = %d\n", ret);
> > > > + return ret;
> > > > + }
> > > > +
> > > > + ret = clk_bulk_prepare_enable(variant->num_clks, plat->clks);
> > > > + if (ret) {
> > > > + dev_err(plat->dev, "failed to enable clks, err = %d\n", ret);
> > > > + return ret;
> > > > + }
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static void mediatek_dwmac_exit(struct platform_device *pdev, void *priv)
> > > > +{
> > > > + struct mediatek_dwmac_plat_data *plat = priv;
> > > > + const struct mediatek_dwmac_variant *variant = plat->variant;
> > > > +
> > > > + clk_bulk_disable_unprepare(variant->num_clks, plat->clks);
> > > > +}
> > > > +
> > > > +static int mediatek_dwmac_probe(struct platform_device *pdev)
> > > > +{
> > > > + struct mediatek_dwmac_plat_data *priv_plat;
> > > > + struct plat_stmmacenet_data *plat_dat;
> > > > + struct stmmac_resources stmmac_res;
> > > > + int ret = 0;
> > > zero initialized seems unnecessary
> > >
> > ok, will not initialized here
> > > > +
> > > > + priv_plat = devm_kzalloc(&pdev->dev, sizeof(*priv_plat), GFP_KERNEL);
> > > > + if (!priv_plat)
> > > > + return -ENOMEM;
> > > > +
> > > > + priv_plat->variant = of_device_get_match_data(&pdev->dev);
> > > > + if (!priv_plat->variant) {
> > > > + dev_err(&pdev->dev, "Missing dwmac-mediatek variant\n");
> > > > + return -EINVAL;
> > > > + }
> > > > +
> > > > + priv_plat->dev = &pdev->dev;
> > > > + priv_plat->np = pdev->dev.of_node;
> > > > +
> > > > + ret = mediatek_dwmac_config_dt(priv_plat);
> > > > + if (ret)
> > > > + return ret;
> > > > +
> > > > + ret = mediatek_dwmac_clk_init(priv_plat);
> > > > + if (ret)
> > > > + return ret;
> > > > +
> > > > + ret = stmmac_get_platform_resources(pdev, &stmmac_res);
> > > > + if (ret)
> > > > + return ret;
> > > > +
>
> < ... >
^ permalink raw reply
* [PATCH net-next v3 5/5] netns: enable to dump full nsid translation table
From: Nicolas Dichtel @ 2018-11-22 22:22 UTC (permalink / raw)
To: dsahern; +Cc: netdev, davem, Nicolas Dichtel
In-Reply-To: <20181122222215.23554-1-nicolas.dichtel@6wind.com>
Like the previous patch, the goal is to ease to convert nsids from one
netns to another netns.
A new attribute (NETNSA_CURRENT_NSID) is added to the kernel answer when
NETNSA_TARGET_NSID is provided, thus the user can easily convert nsids.
Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---
include/uapi/linux/net_namespace.h | 1 +
net/core/net_namespace.c | 31 ++++++++++++++++++++++++------
2 files changed, 26 insertions(+), 6 deletions(-)
diff --git a/include/uapi/linux/net_namespace.h b/include/uapi/linux/net_namespace.h
index 0ed9dd61d32a..9f9956809565 100644
--- a/include/uapi/linux/net_namespace.h
+++ b/include/uapi/linux/net_namespace.h
@@ -17,6 +17,7 @@ enum {
NETNSA_PID,
NETNSA_FD,
NETNSA_TARGET_NSID,
+ NETNSA_CURRENT_NSID,
__NETNSA_MAX,
};
diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c
index dd25fb22ad45..2f25d7f2a43b 100644
--- a/net/core/net_namespace.c
+++ b/net/core/net_namespace.c
@@ -736,6 +736,7 @@ static int rtnl_net_get_size(void)
{
return NLMSG_ALIGN(sizeof(struct rtgenmsg))
+ nla_total_size(sizeof(s32)) /* NETNSA_NSID */
+ + nla_total_size(sizeof(s32)) /* NETNSA_CURRENT_NSID */
;
}
@@ -745,6 +746,8 @@ struct net_fill_args {
int flags;
int cmd;
int nsid;
+ bool add_ref;
+ int ref_nsid;
};
static int rtnl_net_fill(struct sk_buff *skb, struct net_fill_args *args)
@@ -763,6 +766,10 @@ static int rtnl_net_fill(struct sk_buff *skb, struct net_fill_args *args)
if (nla_put_s32(skb, NETNSA_NSID, args->nsid))
goto nla_put_failure;
+ if (args->add_ref &&
+ nla_put_s32(skb, NETNSA_CURRENT_NSID, args->ref_nsid))
+ goto nla_put_failure;
+
nlmsg_end(skb, nlh);
return 0;
@@ -782,7 +789,6 @@ static int rtnl_net_getid(struct sk_buff *skb, struct nlmsghdr *nlh,
.cmd = RTM_NEWNSID,
};
struct net *peer, *target = net;
- bool put_target = false;
struct nlattr *nla;
struct sk_buff *msg;
int err;
@@ -824,7 +830,8 @@ static int rtnl_net_getid(struct sk_buff *skb, struct nlmsghdr *nlh,
err = PTR_ERR(target);
goto out;
}
- put_target = true;
+ fillargs.add_ref = true;
+ fillargs.ref_nsid = peernet2id(net, peer);
}
msg = nlmsg_new(rtnl_net_get_size(), GFP_KERNEL);
@@ -844,7 +851,7 @@ static int rtnl_net_getid(struct sk_buff *skb, struct nlmsghdr *nlh,
err_out:
nlmsg_free(msg);
out:
- if (put_target)
+ if (fillargs.add_ref)
put_net(target);
put_net(peer);
return err;
@@ -852,11 +859,11 @@ static int rtnl_net_getid(struct sk_buff *skb, struct nlmsghdr *nlh,
struct rtnl_net_dump_cb {
struct net *tgt_net;
+ struct net *ref_net;
struct sk_buff *skb;
struct net_fill_args fillargs;
int idx;
int s_idx;
- bool put_tgt_net;
};
static int rtnl_net_dumpid_one(int id, void *peer, void *data)
@@ -868,6 +875,8 @@ static int rtnl_net_dumpid_one(int id, void *peer, void *data)
goto cont;
net_cb->fillargs.nsid = id;
+ if (net_cb->fillargs.add_ref)
+ net_cb->fillargs.ref_nsid = __peernet2id(net_cb->ref_net, peer);
ret = rtnl_net_fill(net_cb->skb, &net_cb->fillargs);
if (ret < 0)
return ret;
@@ -904,8 +913,9 @@ static int rtnl_valid_dump_net_req(const struct nlmsghdr *nlh, struct sock *sk,
"Invalid target network namespace id");
return PTR_ERR(net);
}
+ net_cb->fillargs.add_ref = true;
+ net_cb->ref_net = net_cb->tgt_net;
net_cb->tgt_net = net;
- net_cb->put_tgt_net = true;
} else {
NL_SET_BAD_ATTR(extack, tb[i]);
NL_SET_ERR_MSG(extack,
@@ -940,12 +950,21 @@ static int rtnl_net_dumpid(struct sk_buff *skb, struct netlink_callback *cb)
}
spin_lock_bh(&net_cb.tgt_net->nsid_lock);
+ if (net_cb.fillargs.add_ref &&
+ !net_eq(net_cb.ref_net, net_cb.tgt_net) &&
+ !spin_trylock_bh(&net_cb.ref_net->nsid_lock)) {
+ err = -EAGAIN;
+ goto end;
+ }
idr_for_each(&net_cb.tgt_net->netns_ids, rtnl_net_dumpid_one, &net_cb);
+ if (net_cb.fillargs.add_ref &&
+ !net_eq(net_cb.ref_net, net_cb.tgt_net))
+ spin_unlock_bh(&net_cb.ref_net->nsid_lock);
spin_unlock_bh(&net_cb.tgt_net->nsid_lock);
cb->args[0] = net_cb.idx;
end:
- if (net_cb.put_tgt_net)
+ if (net_cb.fillargs.add_ref)
put_net(net_cb.tgt_net);
return err < 0 ? err : skb->len;
}
--
2.18.0
^ permalink raw reply related
* [PATCH net-next v3 3/5] netns: add support of NETNSA_TARGET_NSID
From: Nicolas Dichtel @ 2018-11-22 22:22 UTC (permalink / raw)
To: dsahern; +Cc: netdev, davem, Nicolas Dichtel
In-Reply-To: <20181122222215.23554-1-nicolas.dichtel@6wind.com>
Like it was done for link and address, add the ability to perform get/dump
in another netns by specifying a target nsid attribute.
Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
Reviewed-by: David Ahern <dsahern@gmail.com>
---
include/uapi/linux/net_namespace.h | 1 +
net/core/net_namespace.c | 86 ++++++++++++++++++++++++++----
2 files changed, 76 insertions(+), 11 deletions(-)
diff --git a/include/uapi/linux/net_namespace.h b/include/uapi/linux/net_namespace.h
index 0187c74d8889..0ed9dd61d32a 100644
--- a/include/uapi/linux/net_namespace.h
+++ b/include/uapi/linux/net_namespace.h
@@ -16,6 +16,7 @@ enum {
NETNSA_NSID,
NETNSA_PID,
NETNSA_FD,
+ NETNSA_TARGET_NSID,
__NETNSA_MAX,
};
diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c
index f8a5966b086c..885c54197e31 100644
--- a/net/core/net_namespace.c
+++ b/net/core/net_namespace.c
@@ -669,6 +669,7 @@ static const struct nla_policy rtnl_net_policy[NETNSA_MAX + 1] = {
[NETNSA_NSID] = { .type = NLA_S32 },
[NETNSA_PID] = { .type = NLA_U32 },
[NETNSA_FD] = { .type = NLA_U32 },
+ [NETNSA_TARGET_NSID] = { .type = NLA_S32 },
};
static int rtnl_net_newid(struct sk_buff *skb, struct nlmsghdr *nlh,
@@ -780,9 +781,10 @@ static int rtnl_net_getid(struct sk_buff *skb, struct nlmsghdr *nlh,
.seq = nlh->nlmsg_seq,
.cmd = RTM_NEWNSID,
};
+ struct net *peer, *target = net;
+ bool put_target = false;
struct nlattr *nla;
struct sk_buff *msg;
- struct net *peer;
int err;
err = nlmsg_parse(nlh, sizeof(struct rtgenmsg), tb, NETNSA_MAX,
@@ -806,13 +808,27 @@ static int rtnl_net_getid(struct sk_buff *skb, struct nlmsghdr *nlh,
return PTR_ERR(peer);
}
+ if (tb[NETNSA_TARGET_NSID]) {
+ int id = nla_get_s32(tb[NETNSA_TARGET_NSID]);
+
+ target = rtnl_get_net_ns_capable(NETLINK_CB(skb).sk, id);
+ if (IS_ERR(target)) {
+ NL_SET_BAD_ATTR(extack, tb[NETNSA_TARGET_NSID]);
+ NL_SET_ERR_MSG(extack,
+ "Target netns reference is invalid");
+ err = PTR_ERR(target);
+ goto out;
+ }
+ put_target = true;
+ }
+
msg = nlmsg_new(rtnl_net_get_size(), GFP_KERNEL);
if (!msg) {
err = -ENOMEM;
goto out;
}
- fillargs.nsid = peernet2id(net, peer);
+ fillargs.nsid = peernet2id(target, peer);
err = rtnl_net_fill(msg, &fillargs);
if (err < 0)
goto err_out;
@@ -823,15 +839,19 @@ static int rtnl_net_getid(struct sk_buff *skb, struct nlmsghdr *nlh,
err_out:
nlmsg_free(msg);
out:
+ if (put_target)
+ put_net(target);
put_net(peer);
return err;
}
struct rtnl_net_dump_cb {
+ struct net *tgt_net;
struct sk_buff *skb;
struct net_fill_args fillargs;
int idx;
int s_idx;
+ bool put_tgt_net;
};
static int rtnl_net_dumpid_one(int id, void *peer, void *data)
@@ -852,10 +872,50 @@ static int rtnl_net_dumpid_one(int id, void *peer, void *data)
return 0;
}
+static int rtnl_valid_dump_net_req(const struct nlmsghdr *nlh, struct sock *sk,
+ struct rtnl_net_dump_cb *net_cb,
+ struct netlink_callback *cb)
+{
+ struct netlink_ext_ack *extack = cb->extack;
+ struct nlattr *tb[NETNSA_MAX + 1];
+ int err, i;
+
+ err = nlmsg_parse_strict(nlh, sizeof(struct rtgenmsg), tb, NETNSA_MAX,
+ rtnl_net_policy, extack);
+ if (err < 0)
+ return err;
+
+ for (i = 0; i <= NETNSA_MAX; i++) {
+ if (!tb[i])
+ continue;
+
+ if (i == NETNSA_TARGET_NSID) {
+ struct net *net;
+
+ net = rtnl_get_net_ns_capable(sk, nla_get_s32(tb[i]));
+ if (IS_ERR(net)) {
+ NL_SET_BAD_ATTR(extack, tb[i]);
+ NL_SET_ERR_MSG(extack,
+ "Invalid target network namespace id");
+ return PTR_ERR(net);
+ }
+ net_cb->tgt_net = net;
+ net_cb->put_tgt_net = true;
+ } else {
+ NL_SET_BAD_ATTR(extack, tb[i]);
+ NL_SET_ERR_MSG(extack,
+ "Unsupported attribute in dump request");
+ return -EINVAL;
+ }
+ }
+
+ return 0;
+}
+
static int rtnl_net_dumpid(struct sk_buff *skb, struct netlink_callback *cb)
{
- struct net *net = sock_net(skb->sk);
struct rtnl_net_dump_cb net_cb = {
+ .tgt_net = sock_net(skb->sk),
.skb = skb,
.fillargs = {
.portid = NETLINK_CB(cb->skb).portid,
@@ -866,19 +926,23 @@ static int rtnl_net_dumpid(struct sk_buff *skb, struct netlink_callback *cb)
.idx = 0,
.s_idx = cb->args[0],
};
+ int err = 0;
- if (cb->strict_check &&
- nlmsg_attrlen(cb->nlh, sizeof(struct rtgenmsg))) {
- NL_SET_ERR_MSG(cb->extack, "Unknown data in network namespace id dump request");
- return -EINVAL;
+ if (cb->strict_check) {
+ err = rtnl_valid_dump_net_req(cb->nlh, skb->sk, &net_cb, cb);
+ if (err < 0)
+ goto end;
}
- spin_lock_bh(&net->nsid_lock);
- idr_for_each(&net->netns_ids, rtnl_net_dumpid_one, &net_cb);
- spin_unlock_bh(&net->nsid_lock);
+ spin_lock_bh(&net_cb.tgt_net->nsid_lock);
+ idr_for_each(&net_cb.tgt_net->netns_ids, rtnl_net_dumpid_one, &net_cb);
+ spin_unlock_bh(&net_cb.tgt_net->nsid_lock);
cb->args[0] = net_cb.idx;
- return skb->len;
+end:
+ if (net_cb.put_tgt_net)
+ put_net(net_cb.tgt_net);
+ return err < 0 ? err : skb->len;
}
static void rtnl_net_notifyid(struct net *net, int cmd, int id)
--
2.18.0
^ permalink raw reply related
* [PATCH net-next v3 1/5] netns: remove net arg from rtnl_net_fill()
From: Nicolas Dichtel @ 2018-11-22 22:22 UTC (permalink / raw)
To: dsahern; +Cc: netdev, davem, Nicolas Dichtel
In-Reply-To: <20181122222215.23554-1-nicolas.dichtel@6wind.com>
This argument is not used anymore.
Fixes: cab3c8ec8d57 ("netns: always provide the id to rtnl_net_fill()")
Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
Reviewed-by: David Ahern <dsahern@gmail.com>
---
net/core/net_namespace.c | 10 ++++------
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c
index fefe72774aeb..52b9620e3457 100644
--- a/net/core/net_namespace.c
+++ b/net/core/net_namespace.c
@@ -739,7 +739,7 @@ static int rtnl_net_get_size(void)
}
static int rtnl_net_fill(struct sk_buff *skb, u32 portid, u32 seq, int flags,
- int cmd, struct net *net, int nsid)
+ int cmd, int nsid)
{
struct nlmsghdr *nlh;
struct rtgenmsg *rth;
@@ -801,7 +801,7 @@ static int rtnl_net_getid(struct sk_buff *skb, struct nlmsghdr *nlh,
id = peernet2id(net, peer);
err = rtnl_net_fill(msg, NETLINK_CB(skb).portid, nlh->nlmsg_seq, 0,
- RTM_NEWNSID, net, id);
+ RTM_NEWNSID, id);
if (err < 0)
goto err_out;
@@ -816,7 +816,6 @@ static int rtnl_net_getid(struct sk_buff *skb, struct nlmsghdr *nlh,
}
struct rtnl_net_dump_cb {
- struct net *net;
struct sk_buff *skb;
struct netlink_callback *cb;
int idx;
@@ -833,7 +832,7 @@ static int rtnl_net_dumpid_one(int id, void *peer, void *data)
ret = rtnl_net_fill(net_cb->skb, NETLINK_CB(net_cb->cb->skb).portid,
net_cb->cb->nlh->nlmsg_seq, NLM_F_MULTI,
- RTM_NEWNSID, net_cb->net, id);
+ RTM_NEWNSID, id);
if (ret < 0)
return ret;
@@ -846,7 +845,6 @@ static int rtnl_net_dumpid(struct sk_buff *skb, struct netlink_callback *cb)
{
struct net *net = sock_net(skb->sk);
struct rtnl_net_dump_cb net_cb = {
- .net = net,
.skb = skb,
.cb = cb,
.idx = 0,
@@ -876,7 +874,7 @@ static void rtnl_net_notifyid(struct net *net, int cmd, int id)
if (!msg)
goto out;
- err = rtnl_net_fill(msg, 0, 0, 0, cmd, net, id);
+ err = rtnl_net_fill(msg, 0, 0, 0, cmd, id);
if (err < 0)
goto err_out;
--
2.18.0
^ permalink raw reply related
* [PATCH net-next v3 4/5] netns: enable to specify a nsid for a get request
From: Nicolas Dichtel @ 2018-11-22 22:22 UTC (permalink / raw)
To: dsahern; +Cc: netdev, davem, Nicolas Dichtel
In-Reply-To: <20181122222215.23554-1-nicolas.dichtel@6wind.com>
Combined with NETNSA_TARGET_NSID, it enables to "translate" a nsid from one
netns to a nsid of another netns.
This is useful when using NETLINK_F_LISTEN_ALL_NSID because it helps the
user to interpret a nsid received from an other netns.
Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
Reviewed-by: David Ahern <dsahern@gmail.com>
---
net/core/net_namespace.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c
index 885c54197e31..dd25fb22ad45 100644
--- a/net/core/net_namespace.c
+++ b/net/core/net_namespace.c
@@ -797,6 +797,11 @@ static int rtnl_net_getid(struct sk_buff *skb, struct nlmsghdr *nlh,
} else if (tb[NETNSA_FD]) {
peer = get_net_ns_by_fd(nla_get_u32(tb[NETNSA_FD]));
nla = tb[NETNSA_FD];
+ } else if (tb[NETNSA_NSID]) {
+ peer = get_net_ns_by_id(net, nla_get_u32(tb[NETNSA_NSID]));
+ if (!peer)
+ peer = ERR_PTR(-ENOENT);
+ nla = tb[NETNSA_NSID];
} else {
NL_SET_ERR_MSG(extack, "Peer netns reference is missing");
return -EINVAL;
--
2.18.0
^ permalink raw reply related
* [PATCH net-next v3 0/5] Ease to interpret net-nsid
From: Nicolas Dichtel @ 2018-11-22 22:22 UTC (permalink / raw)
To: dsahern; +Cc: netdev, davem
In-Reply-To: <20181122155031.3495-1-nicolas.dichtel@6wind.com>
The goal of this series is to ease the interpretation of nsid received in
netlink messages from other netns (when the user uses
NETLINK_F_LISTEN_ALL_NSID).
After this series, with a patched iproute2:
$ ip netns add foo
$ ip netns add bar
$ touch /var/run/netns/init_net
$ mount --bind /proc/1/ns/net /var/run/netns/init_net
$ ip netns set init_net 11
$ ip netns set foo 12
$ ip netns set bar 13
$ ip netns
init_net (id: 11)
bar (id: 13)
foo (id: 12)
$ ip -n foo netns set init_net 21
$ ip -n foo netns set foo 22
$ ip -n foo netns set bar 23
$ ip -n foo netns
init_net (id: 21)
bar (id: 23)
foo (id: 22)
$ ip -n bar netns set init_net 31
$ ip -n bar netns set foo 32
$ ip -n bar netns set bar 33
$ ip -n bar netns
init_net (id: 31)
bar (id: 33)
foo (id: 32)
$ ip netns list-id target-nsid 12
nsid 21 current-nsid 11 (iproute2 netns name: init_net)
nsid 22 current-nsid 12 (iproute2 netns name: foo)
nsid 23 current-nsid 13 (iproute2 netns name: bar)
$ ip -n bar netns list-id target-nsid 32 nsid 31
nsid 21 current-nsid 31 (iproute2 netns name: init_net)
v2 -> v3:
- patch 5/5: account NETNSA_CURRENT_NSID in rtnl_net_get_size()
v1 -> v2:
- patch 1/5: remove net from struct rtnl_net_dump_cb
- patch 2/5: new in this version
- patch 3/5: use a bool to know if rtnl_get_net_ns_capable() was called
- patch 5/5: use struct net_fill_args
include/uapi/linux/net_namespace.h | 2 +
net/core/net_namespace.c | 158 +++++++++++++++++++++++++++++++------
2 files changed, 134 insertions(+), 26 deletions(-)
Comments are welcomed,
Regards,
Nicolas
^ permalink raw reply
* [PATCH net-next v3 2/5] netns: introduce 'struct net_fill_args'
From: Nicolas Dichtel @ 2018-11-22 22:22 UTC (permalink / raw)
To: dsahern; +Cc: netdev, davem, Nicolas Dichtel
In-Reply-To: <20181122222215.23554-1-nicolas.dichtel@6wind.com>
This is a preparatory work. To avoid having to much arguments for the
function rtnl_net_fill(), a new structure is defined.
Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
Reviewed-by: David Ahern <dsahern@gmail.com>
---
net/core/net_namespace.c | 48 ++++++++++++++++++++++++++++------------
1 file changed, 34 insertions(+), 14 deletions(-)
diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c
index 52b9620e3457..f8a5966b086c 100644
--- a/net/core/net_namespace.c
+++ b/net/core/net_namespace.c
@@ -738,20 +738,28 @@ static int rtnl_net_get_size(void)
;
}
-static int rtnl_net_fill(struct sk_buff *skb, u32 portid, u32 seq, int flags,
- int cmd, int nsid)
+struct net_fill_args {
+ u32 portid;
+ u32 seq;
+ int flags;
+ int cmd;
+ int nsid;
+};
+
+static int rtnl_net_fill(struct sk_buff *skb, struct net_fill_args *args)
{
struct nlmsghdr *nlh;
struct rtgenmsg *rth;
- nlh = nlmsg_put(skb, portid, seq, cmd, sizeof(*rth), flags);
+ nlh = nlmsg_put(skb, args->portid, args->seq, args->cmd, sizeof(*rth),
+ args->flags);
if (!nlh)
return -EMSGSIZE;
rth = nlmsg_data(nlh);
rth->rtgen_family = AF_UNSPEC;
- if (nla_put_s32(skb, NETNSA_NSID, nsid))
+ if (nla_put_s32(skb, NETNSA_NSID, args->nsid))
goto nla_put_failure;
nlmsg_end(skb, nlh);
@@ -767,10 +775,15 @@ static int rtnl_net_getid(struct sk_buff *skb, struct nlmsghdr *nlh,
{
struct net *net = sock_net(skb->sk);
struct nlattr *tb[NETNSA_MAX + 1];
+ struct net_fill_args fillargs = {
+ .portid = NETLINK_CB(skb).portid,
+ .seq = nlh->nlmsg_seq,
+ .cmd = RTM_NEWNSID,
+ };
struct nlattr *nla;
struct sk_buff *msg;
struct net *peer;
- int err, id;
+ int err;
err = nlmsg_parse(nlh, sizeof(struct rtgenmsg), tb, NETNSA_MAX,
rtnl_net_policy, extack);
@@ -799,9 +812,8 @@ static int rtnl_net_getid(struct sk_buff *skb, struct nlmsghdr *nlh,
goto out;
}
- id = peernet2id(net, peer);
- err = rtnl_net_fill(msg, NETLINK_CB(skb).portid, nlh->nlmsg_seq, 0,
- RTM_NEWNSID, id);
+ fillargs.nsid = peernet2id(net, peer);
+ err = rtnl_net_fill(msg, &fillargs);
if (err < 0)
goto err_out;
@@ -817,7 +829,7 @@ static int rtnl_net_getid(struct sk_buff *skb, struct nlmsghdr *nlh,
struct rtnl_net_dump_cb {
struct sk_buff *skb;
- struct netlink_callback *cb;
+ struct net_fill_args fillargs;
int idx;
int s_idx;
};
@@ -830,9 +842,8 @@ static int rtnl_net_dumpid_one(int id, void *peer, void *data)
if (net_cb->idx < net_cb->s_idx)
goto cont;
- ret = rtnl_net_fill(net_cb->skb, NETLINK_CB(net_cb->cb->skb).portid,
- net_cb->cb->nlh->nlmsg_seq, NLM_F_MULTI,
- RTM_NEWNSID, id);
+ net_cb->fillargs.nsid = id;
+ ret = rtnl_net_fill(net_cb->skb, &net_cb->fillargs);
if (ret < 0)
return ret;
@@ -846,7 +857,12 @@ static int rtnl_net_dumpid(struct sk_buff *skb, struct netlink_callback *cb)
struct net *net = sock_net(skb->sk);
struct rtnl_net_dump_cb net_cb = {
.skb = skb,
- .cb = cb,
+ .fillargs = {
+ .portid = NETLINK_CB(cb->skb).portid,
+ .seq = cb->nlh->nlmsg_seq,
+ .flags = NLM_F_MULTI,
+ .cmd = RTM_NEWNSID,
+ },
.idx = 0,
.s_idx = cb->args[0],
};
@@ -867,6 +883,10 @@ static int rtnl_net_dumpid(struct sk_buff *skb, struct netlink_callback *cb)
static void rtnl_net_notifyid(struct net *net, int cmd, int id)
{
+ struct net_fill_args fillargs = {
+ .cmd = cmd,
+ .nsid = id,
+ };
struct sk_buff *msg;
int err = -ENOMEM;
@@ -874,7 +894,7 @@ static void rtnl_net_notifyid(struct net *net, int cmd, int id)
if (!msg)
goto out;
- err = rtnl_net_fill(msg, 0, 0, 0, cmd, id);
+ err = rtnl_net_fill(msg, &fillargs);
if (err < 0)
goto err_out;
--
2.18.0
^ permalink raw reply related
* [PATCH v2] samples: bpf: fix: error handling regarding kprobe_events
From: Daniel T. Lee @ 2018-11-22 22:14 UTC (permalink / raw)
To: Daniel Borkmann, Alexei Starovoitov; +Cc: netdev
Currently, kprobe_events failure won't be handled properly.
Due to calling system() indirectly to write to kprobe_events,
it can't be identified whether an error is derived from kprobe or system.
// buf = "echo '%c:%s %s' >> /s/k/d/t/kprobe_events"
err = system(buf);
if (err < 0) {
printf("failed to create kprobe ..");
return -1;
}
For example, running ./tracex7 sample in ext4 partition,
"echo p:open_ctree open_ctree >> /s/k/d/t/kprobe_events"
gets 256 error code system() failure.
=> The error comes from kprobe, but it's not handled correctly.
According to man of system(3), it's return value
just passes the termination status of the child shell
rather than treating the error as -1. (don't care success)
Which means, currently it's not working as desired.
(According to the upper code snippet)
ex) running ./tracex7 with ext4 env.
# Current Output
sh: echo: I/O error
failed to open event open_ctree
# Desired Output
failed to create kprobe 'open_ctree' error 'No such file or directory'
The problem is, error can't be verified whether from child ps or system.
But using write() directly can verify the command failure,
and it will treat all error as -1.
So I suggest using write() directly to 'kprobe_events'
rather than calling system().
Signed-off-by: Daniel T. Lee <danieltimlee@gmail.com>
---
Changes in v2:
- Fix code style at variable declaration.
samples/bpf/bpf_load.c | 33 ++++++++++++++++++++++++---------
1 file changed, 24 insertions(+), 9 deletions(-)
diff --git a/samples/bpf/bpf_load.c b/samples/bpf/bpf_load.c
index e6d7e0fe155b..96783207de4a 100644
--- a/samples/bpf/bpf_load.c
+++ b/samples/bpf/bpf_load.c
@@ -54,6 +54,23 @@ static int populate_prog_array(const char *event, int prog_fd)
return 0;
}
+static int write_kprobe_events(const char *val)
+{
+ int fd, ret, flags;
+
+ if ((val != NULL) && (val[0] == '\0'))
+ flags = O_WRONLY | O_TRUNC;
+ else
+ flags = O_WRONLY | O_APPEND;
+
+ fd = open("/sys/kernel/debug/tracing/kprobe_events", flags);
+
+ ret = write(fd, val, strlen(val));
+ close(fd);
+
+ return ret;
+}
+
static int load_and_attach(const char *event, struct bpf_insn *prog, int size)
{
bool is_socket = strncmp(event, "socket", 6) == 0;
@@ -165,10 +182,9 @@ static int load_and_attach(const char *event, struct bpf_insn *prog, int size)
#ifdef __x86_64__
if (strncmp(event, "sys_", 4) == 0) {
- snprintf(buf, sizeof(buf),
- "echo '%c:__x64_%s __x64_%s' >> /sys/kernel/debug/tracing/kprobe_events",
- is_kprobe ? 'p' : 'r', event, event);
- err = system(buf);
+ snprintf(buf, sizeof(buf), "%c:__x64_%s __x64_%s",
+ is_kprobe ? 'p' : 'r', event, event);
+ err = write_kprobe_events(buf);
if (err >= 0) {
need_normal_check = false;
event_prefix = "__x64_";
@@ -176,10 +192,9 @@ static int load_and_attach(const char *event, struct bpf_insn *prog, int size)
}
#endif
if (need_normal_check) {
- snprintf(buf, sizeof(buf),
- "echo '%c:%s %s' >> /sys/kernel/debug/tracing/kprobe_events",
- is_kprobe ? 'p' : 'r', event, event);
- err = system(buf);
+ snprintf(buf, sizeof(buf), "%c:%s %s",
+ is_kprobe ? 'p' : 'r', event, event);
+ err = write_kprobe_events(buf);
if (err < 0) {
printf("failed to create kprobe '%s' error '%s'\n",
event, strerror(errno));
@@ -519,7 +534,7 @@ static int do_load_bpf_file(const char *path, fixup_map_cb fixup_map)
return 1;
/* clear all kprobes */
- i = system("echo \"\" > /sys/kernel/debug/tracing/kprobe_events");
+ i = write_kprobe_events("");
/* scan over all elf sections to get license and map info */
for (i = 1; i < ehdr.e_shnum; i++) {
--
2.17.1
^ permalink raw reply related
* Re: [RFC PATCH bpf-next] libbpf: make bpf_object__open default to UNSPEC
From: Daniel Borkmann @ 2018-11-22 21:52 UTC (permalink / raw)
To: Nikita V. Shirokov, Alexei Starovoitov; +Cc: netdev, Wang Nan
In-Reply-To: <20181122060303.22143-1-tehnerd@tehnerd.com>
[ +Wang ]
On 11/22/2018 07:03 AM, Nikita V. Shirokov wrote:
> currently by default libbpf's bpf_object__open requires
> bpf's program to specify version in a code because of two things:
> 1) default prog type is set to KPROBE
> 2) KPROBE requires (in kernel/bpf/syscall.c) version to be specified
>
> in this RFC i'm proposing change default to UNSPEC and also changing
> logic of libbpf that it would reflect what we have today in kernel
> (aka only KPROBE type requires for version to be explicitly set).
>
> reason for change:
> currently only libbpf requires by default version to be
> explicitly set. it would be really hard for mainteiners of other custom
> bpf loaders to migrate to libbpf (as they dont control user's code
> and migration to the new loader (libbpf) wont be transparent for end
> user).
>
> what is going to be broken after this change:
> if someone were relying on default to be KPROBE for bpf_object__open
> his code will stop to work. however i'm really doubtfull that anyone
> is using this for kprobe type of programs (instead of, say, bcc or
> other tracing frameworks)
>
> other possible solutions (for discussion, would require more machinery):
> add another function like bpf_object__open w/ default to unspec
>
> Signed-off-by: Nikita V. Shirokov <tehnerd@tehnerd.com>
> ---
> tools/lib/bpf/libbpf.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 0f14f7c074c2..ed4212a4c5f9 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -333,7 +333,7 @@ bpf_program__init(void *data, size_t size, char *section_name, int idx,
> prog->idx = idx;
> prog->instances.fds = NULL;
> prog->instances.nr = -1;
> - prog->type = BPF_PROG_TYPE_KPROBE;
> + prog->type = BPF_PROG_TYPE_UNSPEC;
> prog->btf_fd = -1;
Seems this was mostly for historic reasons, but for a generic library this
would indeed be an odd convention for default. Wang, given 5f44e4c810bf
("tools lib bpf: New API to adjust type of a BPF program"), are you in any
way relying on this default or using things like bpf_program__set_kprobe()
instead which you've added there? If latter, I'd say we should then change
it better now than later when there's even more lib usage (and in particular
before we add official ABI versioning).
> return 0;
> @@ -1649,12 +1649,12 @@ static bool bpf_prog_type__needs_kver(enum bpf_prog_type type)
> case BPF_PROG_TYPE_LIRC_MODE2:
> case BPF_PROG_TYPE_SK_REUSEPORT:
> case BPF_PROG_TYPE_FLOW_DISSECTOR:
> - return false;
> case BPF_PROG_TYPE_UNSPEC:
> - case BPF_PROG_TYPE_KPROBE:
> case BPF_PROG_TYPE_TRACEPOINT:
> - case BPF_PROG_TYPE_PERF_EVENT:
> case BPF_PROG_TYPE_RAW_TRACEPOINT:
> + case BPF_PROG_TYPE_PERF_EVENT:
> + return false;
> + case BPF_PROG_TYPE_KPROBE:
> default:
> return true;
> }
>
Thanks,
Daniel
^ permalink raw reply
* Re: [PATCH net] net: phy: mscc: fix deadlock in vsc85xx_default_config
From: Quentin Schulz @ 2018-11-23 8:18 UTC (permalink / raw)
To: Andrew Lunn
Cc: davem, f.fainelli, allan.nielsen, linux-kernel, netdev,
thomas.petazzoni, alexandre.belloni
In-Reply-To: <20181122192837.GH10697@lunn.ch>
[-- Attachment #1: Type: text/plain, Size: 1889 bytes --]
Hi Andrew,
On Thu, Nov 22, 2018 at 08:28:37PM +0100, Andrew Lunn wrote:
> On Thu, Nov 22, 2018 at 02:12:32PM +0100, Quentin Schulz wrote:
> > The vsc85xx_default_config function called in the vsc85xx_config_init
> > function which is used by VSC8530, VSC8531, VSC8540 and VSC8541 PHYs
> > mistakenly calls phy_read and phy_write in-between phy_select_page and
> > phy_restore_page.
> >
> > phy_select_page and phy_restore_page actually take and release the MDIO
> > bus lock so __phy_write and __phy_read (which assume that you already
> > have the MDIO bus lock unlike phy_write and phy_read) should be used for
> > any call in between the two said functions.
> >
> > Let's fix this deadlock.
> >
> > Fixes: 6a0bfbbe20b0 ("net: phy: mscc: migrate to phy_select/restore_page functions")
> >
> > Signed-off-by: Quentin Schulz <quentin.schulz@bootlin.com>
> > ---
> > drivers/net/phy/mscc.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/phy/mscc.c b/drivers/net/phy/mscc.c
> > index 62269e578718..6856fe4d1a60 100644
> > --- a/drivers/net/phy/mscc.c
> > +++ b/drivers/net/phy/mscc.c
> > @@ -814,10 +814,10 @@ static int vsc85xx_default_config(struct phy_device *phydev)
> > if (rc < 0)
> > goto out_unlock;
> >
> > - reg_val = phy_read(phydev, MSCC_PHY_RGMII_CNTL);
> > + reg_val = __phy_read(phydev, MSCC_PHY_RGMII_CNTL);
> > reg_val &= ~(RGMII_RX_CLK_DELAY_MASK);
> > reg_val |= (RGMII_RX_CLK_DELAY_1_1_NS << RGMII_RX_CLK_DELAY_POS);
> > - phy_write(phydev, MSCC_PHY_RGMII_CNTL, reg_val);
> > + __phy_write(phydev, MSCC_PHY_RGMII_CNTL, reg_val);
>
> Hi Quentin
>
> You appear to be only accessing a single register, read/modify/write.
> I think you can use phy_modify_paged(), which will take care of all
> the locking for you.
>
Grmbl... Of course :)
Thanks,
Quentin
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* [PATCH net v2] net: phy: mscc: fix deadlock in vsc85xx_default_config
From: Quentin Schulz @ 2018-11-23 8:16 UTC (permalink / raw)
To: davem, andrew, f.fainelli
Cc: allan.nielsen, linux-kernel, netdev, thomas.petazzoni,
alexandre.belloni, Quentin Schulz
The vsc85xx_default_config function called in the vsc85xx_config_init
function which is used by VSC8530, VSC8531, VSC8540 and VSC8541 PHYs
mistakenly calls phy_read and phy_write in-between phy_select_page and
phy_restore_page.
phy_select_page and phy_restore_page actually take and release the MDIO
bus lock and phy_write and phy_read take and release the lock to write
or read to a PHY register.
Let's fix this deadlock by using phy_modify_paged which handles
correctly a read followed by a write in a non-standard page.
Fixes: 6a0bfbbe20b0 ("net: phy: mscc: migrate to phy_select/restore_page functions")
Signed-off-by: Quentin Schulz <quentin.schulz@bootlin.com>
---
v2:
- use phy_modify_paged instead of
phy_select_page -> __phy_read -> __phy_write -> phy_restore_page
drivers/net/phy/mscc.c | 13 ++++++-------
1 file changed, 6 insertions(+), 7 deletions(-)
diff --git a/drivers/net/phy/mscc.c b/drivers/net/phy/mscc.c
index 62269e578718..4dcf7ad06259 100644
--- a/drivers/net/phy/mscc.c
+++ b/drivers/net/phy/mscc.c
@@ -810,17 +810,16 @@ static int vsc85xx_default_config(struct phy_device *phydev)
phydev->mdix_ctrl = ETH_TP_MDI_AUTO;
mutex_lock(&phydev->lock);
- rc = phy_select_page(phydev, MSCC_PHY_PAGE_EXTENDED_2);
+
+ reg_val = RGMII_RX_CLK_DELAY_1_1_NS << RGMII_RX_CLK_DELAY_POS;
+
+ rc = phy_modify_paged(phydev, MSCC_PHY_PAGE_EXTENDED_2,
+ MSCC_PHY_RGMII_CNTL, RGMII_RX_CLK_DELAY_MASK,
+ reg_val);
if (rc < 0)
goto out_unlock;
- reg_val = phy_read(phydev, MSCC_PHY_RGMII_CNTL);
- reg_val &= ~(RGMII_RX_CLK_DELAY_MASK);
- reg_val |= (RGMII_RX_CLK_DELAY_1_1_NS << RGMII_RX_CLK_DELAY_POS);
- phy_write(phydev, MSCC_PHY_RGMII_CNTL, reg_val);
-
out_unlock:
- rc = phy_restore_page(phydev, rc, rc > 0 ? 0 : rc);
mutex_unlock(&phydev->lock);
return rc;
--
2.17.1
^ permalink raw reply related
* Re: [PATCH] bpf: fix check of allowed specifiers in bpf_trace_printk
From: Daniel Borkmann @ 2018-11-22 21:31 UTC (permalink / raw)
To: Martynas Pumputis, netdev; +Cc: ast
In-Reply-To: <20181122160030.15954-1-m@lambda.lt>
Hi Martynas,
On 11/22/2018 05:00 PM, Martynas Pumputis wrote:
> A format string consisting of "%p" or "%s" followed by an invalid
> specifier (e.g. "%p%\n" or "%s%") could pass the check which
> would make format_decode (lib/vsprintf.c) to warn.
>
> Reported-by: syzbot+1ec5c5ec949c4adaa0c4@syzkaller.appspotmail.com
> Signed-off-by: Martynas Pumputis <m@lambda.lt>
> ---
> kernel/trace/bpf_trace.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index 08fcfe440c63..9ab05736e1a1 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -225,6 +225,8 @@ BPF_CALL_5(bpf_trace_printk, char *, fmt, u32, fmt_size, u64, arg1,
> (void *) (long) unsafe_addr,
> sizeof(buf));
> }
> + if (fmt[i] == '%')
> + i--;
> continue;
> }
Thanks for the fix! Could we simplify the logic a bit to avoid having to
navigate i back and forth which got us in trouble in the first place? Like
below (untested) perhaps?
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index 08fcfe4..ff83b8c 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -196,11 +196,13 @@ BPF_CALL_5(bpf_trace_printk, char *, fmt, u32, fmt_size, u64, arg1,
i++;
} else if (fmt[i] == 'p' || fmt[i] == 's') {
mod[fmt_cnt]++;
- i++;
- if (!isspace(fmt[i]) && !ispunct(fmt[i]) && fmt[i] != 0)
+ /* Disallow any further format extensions. */
+ if (fmt[i + 1] != 0 &&
+ !isspace(fmt[i + 1]) &&
+ !ispunct(fmt[i + 1]))
return -EINVAL;
fmt_cnt++;
- if (fmt[i - 1] == 's') {
+ if (fmt[i] == 's') {
if (str_seen)
/* allow only one '%s' per fmt string */
return -EINVAL;
Thanks,
Daniel
^ permalink raw reply related
* Re: [PATCH net-next,v3 00/12] add flow_rule infrastructure
From: Marcelo Ricardo Leitner @ 2018-11-22 21:08 UTC (permalink / raw)
To: Pablo Neira Ayuso
Cc: netdev, davem, thomas.lendacky, f.fainelli, ariel.elior,
michael.chan, santosh, madalin.bucur, yisen.zhuang, salil.mehta,
jeffrey.t.kirsher, tariqt, saeedm, jiri, idosch, jakub.kicinski,
peppe.cavallaro, grygorii.strashko, andrew, vivien.didelot,
alexandre.torgue, joabreu, linux-net-drivers, ganeshgr, ogerlitz,
Manish.Chopra
In-Reply-To: <20181122162220.GB8353@localhost.localdomain>
On Thu, Nov 22, 2018 at 02:22:20PM -0200, Marcelo Ricardo Leitner wrote:
> On Wed, Nov 21, 2018 at 03:51:20AM +0100, Pablo Neira Ayuso wrote:
> > Hi,
> >
> > This patchset is the third iteration [1] [2] [3] to introduce a kernel
> > intermediate (IR) to express ACL hardware offloads.
>
> On v2 cover letter you had:
>
> """
> However, cost of this layer is very small, adding 1 million rules via
> tc -batch, perf shows:
>
> 0.06% tc [kernel.vmlinux] [k] tc_setup_flow_action
> """
>
> The above doesn't include time spent on children calls and I'm worried
> about the new allocation done by flow_rule_alloc(), as it can impact
> rule insertion rate. I'll run some tests here and report back.
I'm seeing +60ms on 1.75s (~3.4%) to add 40k flower rules on ingress
with skip_hw and tc in batch mode, with flows like:
filter add dev p6p2 parent ffff: protocol ip prio 1 flower skip_hw
src_mac ec:13:db:00:00:00 dst_mac ec:14:c2:00:00:00 src_ip
56.0.0.0 dst_ip 55.0.0.0 action drop
Only 20ms out of those 60ms were consumed within fl_change() calls
(considering children calls), though.
Do you see something similar? I used current net-next (d59da3fbfe3f)
and with this patchset applied.
^ permalink raw reply
* [PATCH net-next 5/5] r8169: replace macro TX_FRAGS_READY_FOR with a function
From: Heiner Kallweit @ 2018-11-22 21:03 UTC (permalink / raw)
To: Realtek linux nic maintainers, David Miller; +Cc: netdev@vger.kernel.org
In-Reply-To: <79f821cf-2ad4-98f0-558d-eda3148cec38@gmail.com>
Replace macro TX_FRAGS_READY_FOR with function rtl_tx_slots_avail
to make code cleaner and type-safe.
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
drivers/net/ethernet/realtek/r8169.c | 24 +++++++++++++-----------
1 file changed, 13 insertions(+), 11 deletions(-)
diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index 2ca0b2ed9..f768b966e 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -56,13 +56,6 @@
#define R8169_MSG_DEFAULT \
(NETIF_MSG_DRV | NETIF_MSG_PROBE | NETIF_MSG_IFUP | NETIF_MSG_IFDOWN)
-#define TX_SLOTS_AVAIL(tp) \
- (tp->dirty_tx + NUM_TX_DESC - tp->cur_tx)
-
-/* A skbuff with nr_frags needs nr_frags+1 entries in the tx queue */
-#define TX_FRAGS_READY_FOR(tp,nr_frags) \
- (TX_SLOTS_AVAIL(tp) >= (nr_frags + 1))
-
/* Maximum number of multicast addresses to filter (vs. Rx-all-multicast).
The RTL chips use a 64 element hash table based on the Ethernet CRC. */
static const int multicast_filter_limit = 32;
@@ -6058,6 +6051,15 @@ static bool rtl8169_tso_csum_v2(struct rtl8169_private *tp,
return true;
}
+static bool rtl_tx_slots_avail(struct rtl8169_private *tp,
+ unsigned int nr_frags)
+{
+ unsigned int slots_avail = tp->dirty_tx + NUM_TX_DESC - tp->cur_tx;
+
+ /* A skbuff with nr_frags needs nr_frags+1 entries in the tx queue */
+ return slots_avail > nr_frags;
+}
+
static netdev_tx_t rtl8169_start_xmit(struct sk_buff *skb,
struct net_device *dev)
{
@@ -6069,7 +6071,7 @@ static netdev_tx_t rtl8169_start_xmit(struct sk_buff *skb,
u32 opts[2], len;
int frags;
- if (unlikely(!TX_FRAGS_READY_FOR(tp, skb_shinfo(skb)->nr_frags))) {
+ if (unlikely(!rtl_tx_slots_avail(tp, skb_shinfo(skb)->nr_frags))) {
netif_err(tp, drv, dev, "BUG! Tx Ring full when queue awake!\n");
goto err_stop_0;
}
@@ -6126,7 +6128,7 @@ static netdev_tx_t rtl8169_start_xmit(struct sk_buff *skb,
mmiowb();
- if (!TX_FRAGS_READY_FOR(tp, MAX_SKB_FRAGS)) {
+ if (!rtl_tx_slots_avail(tp, MAX_SKB_FRAGS)) {
/* Avoid wrongly optimistic queue wake-up: rtl_tx thread must
* not miss a ring update when it notices a stopped queue.
*/
@@ -6140,7 +6142,7 @@ static netdev_tx_t rtl8169_start_xmit(struct sk_buff *skb,
* can't.
*/
smp_mb();
- if (TX_FRAGS_READY_FOR(tp, MAX_SKB_FRAGS))
+ if (rtl_tx_slots_avail(tp, MAX_SKB_FRAGS))
netif_wake_queue(dev);
}
@@ -6258,7 +6260,7 @@ static void rtl_tx(struct net_device *dev, struct rtl8169_private *tp,
*/
smp_mb();
if (netif_queue_stopped(dev) &&
- TX_FRAGS_READY_FOR(tp, MAX_SKB_FRAGS)) {
+ rtl_tx_slots_avail(tp, MAX_SKB_FRAGS)) {
netif_wake_queue(dev);
}
/*
--
2.19.1
^ permalink raw reply related
* [PATCH net-next 4/5] r8169: use napi_consume_skb where possible
From: Heiner Kallweit @ 2018-11-22 21:02 UTC (permalink / raw)
To: Realtek linux nic maintainers, David Miller; +Cc: netdev@vger.kernel.org
In-Reply-To: <79f821cf-2ad4-98f0-558d-eda3148cec38@gmail.com>
Use napi_consume_skb() where possible to profit from
bulk free infrastructure.
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
drivers/net/ethernet/realtek/r8169.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index 9a696455e..2ca0b2ed9 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -6204,7 +6204,8 @@ static void rtl8169_pcierr_interrupt(struct net_device *dev)
rtl_schedule_task(tp, RTL_FLAG_TASK_RESET_PENDING);
}
-static void rtl_tx(struct net_device *dev, struct rtl8169_private *tp)
+static void rtl_tx(struct net_device *dev, struct rtl8169_private *tp,
+ int budget)
{
unsigned int dirty_tx, tx_left, bytes_compl = 0, pkts_compl = 0;
@@ -6232,7 +6233,7 @@ static void rtl_tx(struct net_device *dev, struct rtl8169_private *tp)
if (status & LastFrag) {
pkts_compl++;
bytes_compl += tx_skb->skb->len;
- dev_consume_skb_any(tx_skb->skb);
+ napi_consume_skb(tx_skb->skb, budget);
tx_skb->skb = NULL;
}
dirty_tx++;
@@ -6475,7 +6476,7 @@ static int rtl8169_poll(struct napi_struct *napi, int budget)
work_done = rtl_rx(dev, tp, (u32) budget);
- rtl_tx(dev, tp);
+ rtl_tx(dev, tp, budget);
if (work_done < budget) {
napi_complete_done(napi, work_done);
--
2.19.1
^ permalink raw reply related
* [PATCH net-next 3/5] r8169: simplify detecting chip versions with same XID
From: Heiner Kallweit @ 2018-11-22 21:00 UTC (permalink / raw)
To: Realtek linux nic maintainers, David Miller; +Cc: netdev@vger.kernel.org
In-Reply-To: <79f821cf-2ad4-98f0-558d-eda3148cec38@gmail.com>
For the GMII chip versions we set the version number which was set
already. This can be simplified.
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
drivers/net/ethernet/realtek/r8169.c | 19 +++++++------------
1 file changed, 7 insertions(+), 12 deletions(-)
diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index 1e549b26b..9a696455e 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -2116,18 +2116,13 @@ static void rtl8169_get_mac_version(struct rtl8169_private *tp)
if (tp->mac_version == RTL_GIGA_MAC_NONE) {
dev_err(tp_to_dev(tp), "unknown chip XID %03x\n", reg & 0xfcf);
- } else if (tp->mac_version == RTL_GIGA_MAC_VER_42) {
- tp->mac_version = tp->supports_gmii ?
- RTL_GIGA_MAC_VER_42 :
- RTL_GIGA_MAC_VER_43;
- } else if (tp->mac_version == RTL_GIGA_MAC_VER_45) {
- tp->mac_version = tp->supports_gmii ?
- RTL_GIGA_MAC_VER_45 :
- RTL_GIGA_MAC_VER_47;
- } else if (tp->mac_version == RTL_GIGA_MAC_VER_46) {
- tp->mac_version = tp->supports_gmii ?
- RTL_GIGA_MAC_VER_46 :
- RTL_GIGA_MAC_VER_48;
+ } else if (!tp->supports_gmii) {
+ if (tp->mac_version == RTL_GIGA_MAC_VER_42)
+ tp->mac_version = RTL_GIGA_MAC_VER_43;
+ else if (tp->mac_version == RTL_GIGA_MAC_VER_45)
+ tp->mac_version = RTL_GIGA_MAC_VER_47;
+ else if (tp->mac_version == RTL_GIGA_MAC_VER_46)
+ tp->mac_version = RTL_GIGA_MAC_VER_48;
}
}
--
2.19.1
^ permalink raw reply related
* [PATCH net-next 2/5] r8169: remove default chip versions
From: Heiner Kallweit @ 2018-11-22 20:58 UTC (permalink / raw)
To: Realtek linux nic maintainers, David Miller; +Cc: netdev@vger.kernel.org
In-Reply-To: <79f821cf-2ad4-98f0-558d-eda3148cec38@gmail.com>
Even the chip versions within a family have so many differences that
using a default chip version doesn't really make sense. Instead
of leaving a best case flaky network connectivity, bail out and
report the unknown chip version.
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
drivers/net/ethernet/realtek/r8169.c | 15 +++++----------
1 file changed, 5 insertions(+), 10 deletions(-)
diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index bef89ba50..1e549b26b 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -2011,8 +2011,7 @@ static const struct ethtool_ops rtl8169_ethtool_ops = {
.set_link_ksettings = phy_ethtool_set_link_ksettings,
};
-static void rtl8169_get_mac_version(struct rtl8169_private *tp,
- u8 default_version)
+static void rtl8169_get_mac_version(struct rtl8169_private *tp)
{
/*
* The driver currently handles the 8168Bf and the 8168Be identically
@@ -2116,9 +2115,7 @@ static void rtl8169_get_mac_version(struct rtl8169_private *tp,
tp->mac_version = p->mac_version;
if (tp->mac_version == RTL_GIGA_MAC_NONE) {
- dev_notice(tp_to_dev(tp),
- "unknown MAC, using family default\n");
- tp->mac_version = default_version;
+ dev_err(tp_to_dev(tp), "unknown chip XID %03x\n", reg & 0xfcf);
} else if (tp->mac_version == RTL_GIGA_MAC_VER_42) {
tp->mac_version = tp->supports_gmii ?
RTL_GIGA_MAC_VER_42 :
@@ -6976,27 +6973,23 @@ static const struct rtl_cfg_info {
u16 irq_mask;
unsigned int has_gmii:1;
const struct rtl_coalesce_info *coalesce_info;
- u8 default_ver;
} rtl_cfg_infos [] = {
[RTL_CFG_0] = {
.hw_start = rtl_hw_start_8169,
.irq_mask = SYSErr | LinkChg | RxOverflow | RxFIFOOver,
.has_gmii = 1,
.coalesce_info = rtl_coalesce_info_8169,
- .default_ver = RTL_GIGA_MAC_VER_01,
},
[RTL_CFG_1] = {
.hw_start = rtl_hw_start_8168,
.irq_mask = LinkChg | RxOverflow,
.has_gmii = 1,
.coalesce_info = rtl_coalesce_info_8168_8136,
- .default_ver = RTL_GIGA_MAC_VER_11,
},
[RTL_CFG_2] = {
.hw_start = rtl_hw_start_8101,
.irq_mask = LinkChg | RxOverflow | RxFIFOOver,
.coalesce_info = rtl_coalesce_info_8168_8136,
- .default_ver = RTL_GIGA_MAC_VER_13,
}
};
@@ -7259,7 +7252,9 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
tp->mmio_addr = pcim_iomap_table(pdev)[region];
/* Identify chip attached to board */
- rtl8169_get_mac_version(tp, cfg->default_ver);
+ rtl8169_get_mac_version(tp);
+ if (tp->mac_version == RTL_GIGA_MAC_NONE)
+ return -ENODEV;
if (rtl_tbi_enabled(tp)) {
dev_err(&pdev->dev, "TBI fiber mode not supported\n");
--
2.19.1
^ permalink raw reply related
* [PATCH net-next 1/5] r8169: remove ancient GCC bug workaround in a second place
From: Heiner Kallweit @ 2018-11-22 20:56 UTC (permalink / raw)
To: Realtek linux nic maintainers, David Miller; +Cc: netdev@vger.kernel.org
In-Reply-To: <79f821cf-2ad4-98f0-558d-eda3148cec38@gmail.com>
Remove ancient GCC bug workaround in a second place and factor out
rtl_8169_get_txd_opts1.
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
drivers/net/ethernet/realtek/r8169.c | 25 ++++++++++++++-----------
1 file changed, 14 insertions(+), 11 deletions(-)
diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index f5781285a..bef89ba50 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -5840,6 +5840,16 @@ static void rtl8169_tx_timeout(struct net_device *dev)
rtl_schedule_task(tp, RTL_FLAG_TASK_RESET_PENDING);
}
+static __le32 rtl8169_get_txd_opts1(u32 opts0, u32 len, unsigned int entry)
+{
+ u32 status = opts0 | len;
+
+ if (entry == NUM_TX_DESC - 1)
+ status |= RingEnd;
+
+ return cpu_to_le32(status);
+}
+
static int rtl8169_xmit_frags(struct rtl8169_private *tp, struct sk_buff *skb,
u32 *opts)
{
@@ -5852,7 +5862,7 @@ static int rtl8169_xmit_frags(struct rtl8169_private *tp, struct sk_buff *skb,
for (cur_frag = 0; cur_frag < info->nr_frags; cur_frag++) {
const skb_frag_t *frag = info->frags + cur_frag;
dma_addr_t mapping;
- u32 status, len;
+ u32 len;
void *addr;
entry = (entry + 1) % NUM_TX_DESC;
@@ -5868,11 +5878,7 @@ static int rtl8169_xmit_frags(struct rtl8169_private *tp, struct sk_buff *skb,
goto err_out;
}
- status = opts[0] | len;
- if (entry == NUM_TX_DESC - 1)
- status |= RingEnd;
-
- txd->opts1 = cpu_to_le32(status);
+ txd->opts1 = rtl8169_get_txd_opts1(opts[0], len, entry);
txd->opts2 = cpu_to_le32(opts[1]);
txd->addr = cpu_to_le64(mapping);
@@ -6068,8 +6074,7 @@ static netdev_tx_t rtl8169_start_xmit(struct sk_buff *skb,
struct TxDesc *txd = tp->TxDescArray + entry;
struct device *d = tp_to_dev(tp);
dma_addr_t mapping;
- u32 status, len;
- u32 opts[2];
+ u32 opts[2], len;
int frags;
if (unlikely(!TX_FRAGS_READY_FOR(tp, skb_shinfo(skb)->nr_frags))) {
@@ -6118,9 +6123,7 @@ static netdev_tx_t rtl8169_start_xmit(struct sk_buff *skb,
/* Force memory writes to complete before releasing descriptor */
dma_wmb();
- /* Anti gcc 2.95.3 bugware (sic) */
- status = opts[0] | len | (RingEnd * !((entry + 1) % NUM_TX_DESC));
- txd->opts1 = cpu_to_le32(status);
+ txd->opts1 = rtl8169_get_txd_opts1(opts[0], len, entry);
/* Force all memory writes to complete before notifying device */
wmb();
--
2.19.1
^ permalink raw reply related
* [PATCH bpf-next] bpf: Add BPF_MAP_TYPE_QUEUE and BPF_MAP_TYPE_QUEUE to bpftool-map
From: David Calavera @ 2018-11-22 20:59 UTC (permalink / raw)
To: Alexei Starovoitov, Daniel Borkmann, netdev
I noticed that these two new BPF Maps are not defined in bpftool.
This patch defines those two maps and adds their names to the
bpftool-map documentation.
Signed-off-by: David Calavera <david.calavera@gmail.com>
---
tools/bpf/bpftool/Documentation/bpftool-map.rst | 3 ++-
tools/bpf/bpftool/map.c | 2 ++
2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/tools/bpf/bpftool/Documentation/bpftool-map.rst b/tools/bpf/bpftool/Documentation/bpftool-map.rst
index f55a2daed59b..9e827e342d9e 100644
--- a/tools/bpf/bpftool/Documentation/bpftool-map.rst
+++ b/tools/bpf/bpftool/Documentation/bpftool-map.rst
@@ -42,7 +42,8 @@ MAP COMMANDS
| | **percpu_array** | **stack_trace** | **cgroup_array** | **lru_hash**
| | **lru_percpu_hash** | **lpm_trie** | **array_of_maps** | **hash_of_maps**
| | **devmap** | **sockmap** | **cpumap** | **xskmap** | **sockhash**
-| | **cgroup_storage** | **reuseport_sockarray** | **percpu_cgroup_storage** }
+| | **cgroup_storage** | **reuseport_sockarray** | **percpu_cgroup_storage**
+| | **queue** | **stack** }
DESCRIPTION
===========
diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
index 7bf38f0e152e..68b656b6edcc 100644
--- a/tools/bpf/bpftool/map.c
+++ b/tools/bpf/bpftool/map.c
@@ -74,6 +74,8 @@ static const char * const map_type_name[] = {
[BPF_MAP_TYPE_CGROUP_STORAGE] = "cgroup_storage",
[BPF_MAP_TYPE_REUSEPORT_SOCKARRAY] = "reuseport_sockarray",
[BPF_MAP_TYPE_PERCPU_CGROUP_STORAGE] = "percpu_cgroup_storage",
+ [BPF_MAP_TYPE_QUEUE] = "queue",
+ [BPF_MAP_TYPE_STACK] = "stack",
};
static bool map_is_per_cpu(__u32 type)
--
2.17.1
^ permalink raw reply related
* [PATCH net-next 0/5] r8169: some functional improvements
From: Heiner Kallweit @ 2018-11-22 20:54 UTC (permalink / raw)
To: Realtek linux nic maintainers, David Miller; +Cc: netdev@vger.kernel.org
This series includes a few functional improvements.
Heiner Kallweit (5):
r8169: Remove ancient GCC bug workaround in a second place
r8169: remove default chip versions
r8169: simplify detecting chip versions with same XID
r8169: use napi_consume_skb where possible
r8169: replace macro TX_FRAGS_READY_FOR with a function
drivers/net/ethernet/realtek/r8169.c | 90 +++++++++++++---------------
1 file changed, 43 insertions(+), 47 deletions(-)
--
2.19.1
^ permalink raw reply
* Re: [PATCH bpf] bpf: fix integer overflow in queue_stack_map
From: Daniel Borkmann @ 2018-11-22 20:38 UTC (permalink / raw)
To: Alexei Starovoitov, David S . Miller
Cc: torvalds, mauricio.vasquez, ww9210, netdev, kernel-team
In-Reply-To: <20181122184956.969118-1-ast@kernel.org>
On 11/22/2018 07:49 PM, Alexei Starovoitov wrote:
> fix the following issues:
> - allow queue_stack_map for root only
> - fix u32 max_entries overflow
> - disallow value_size == 0
>
> Reported-by: Wei Wu <ww9210@gmail.com>
> Fixes: f1a2e44a3aec ("bpf: add queue and stack maps")
> Signed-off-by: Alexei Starovoitov <ast@kernel.org>
Applied, thanks everyone!
^ permalink raw reply
* Re: DSA support for Marvell 88e6065 switch
From: Pavel Machek @ 2018-11-22 20:27 UTC (permalink / raw)
To: Lennert Buytenhek; +Cc: Andrew Lunn, netdev, f.fainelli
In-Reply-To: <20181122153343.GF30556@wantstofly.org>
[-- Attachment #1: Type: text/plain, Size: 2709 bytes --]
Hi!
> > > > If I wanted it to work, what do I need to do? AFAICT phy autoprobing
> > > > should just attach it as soon as it is compiled in?
> > >
> > > Nope. It is a switch, not a PHY. Switches are never auto-probed
> > > because they are not guaranteed to have ID registers.
> > >
> > > You need to use the legacy device tree binding. Look in
> > > Documentation/devicetree/bindings/net/dsa/dsa.txt, section Deprecated
> > > Binding. You can get more examples if you checkout old kernels. Or
> > > kirkwood-rd88f6281.dtsi, the dsa { } node which is disabled.
> >
> > Thanks; I ported code from mv88e66xx in the meantime, and switch
> > appears to be detected.
> >
> > But I'm running into problems with tagging code, and I guess I'd like
> > some help understanding.
> >
> > tag_trailer: allocates new skb, then copies data around.
> >
> > tag_qca: does dev->stats.tx_packets++, and reuses existing skb.
> >
> > tag_brcm: reuses existing skb.
Any idea why tag trailer allocates new skb, and what is going on with
dev->stats.tx_packets++?
> > Is qca wrong in adjusting the statistics? Why does trailer allocate
> > new skb?
> >
> > 6065 seems to use 2-byte header between "SFD" and "Destination
> > address" in the ethernet frame. That's ... strange place to put
> > header, as addresses are now shifted. I need to put ethernet in
> > promisc mode (by running tcpdump) to get data moving.. and can not
> > figure out what to do in tag_...
>
> Does this switch chip not also support trailer mode?
>
> There's basically four tagging modes for Marvell switch chips: header
> mode (the one you described), trailer mode (tag_trailer.c), DSA and
> ethertype DSA. The switch chips I worked on that didn't support
> (ethertype) DSA tagging did support both header and trailer modes,
> and I chose to run them in trailer mode for the reasons you describe
> above, but if your chip doesn't support trailer mode, then yes,
> you'll have to add support for header mode and put the underlying
> interface into promiscuous mode and such.
It seems that 6060 supports both header (probably, parts of docs are
redacted) and trailer mode... but I'm working with 6065. That does not
support trailer mode... or at least word "trailer" does not appear
anywhere in the documentation.
What chip were you working with? I may want to take a look on their
wording.
6065 indeed has some kind of "egress tagging mode" (with four
options), but I have trouble understanding what it really does.
Thanks,
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply
* Re: [v5, PATCH 1/2] net:stmmac: dwmac-mediatek: add support for mt2712
From: Sean Wang @ 2018-11-23 7:04 UTC (permalink / raw)
To: biao.huang
Cc: davem, robh+dt, mark.rutland, devicetree, nelson.chang,
Andrew Lunn, netdev, Liguo Zhang, linux-kernel, Matthias Brugger,
joabreu, linux-mediatek, honghui.zhang, linux-arm-kernel
In-Reply-To: <1542939448.24219.95.camel@mhfsdcap03>
< ... >
> > > + /* select phy interface in top control domain */
> > > + switch (plat->phy_mode) {
> > > + case PHY_INTERFACE_MODE_MII:
> > > + intf_val |= PHY_INTF_MII_GMII;
> > > + break;
> > > + case PHY_INTERFACE_MODE_RMII:
> > > + intf_val |= PHY_INTF_RMII;
> > > + intf_val |= rmii_rxc;
> > how about putting into one line such as intf_val |= (PHY_INTF_RMII | rmii_rxc) ?
> >
> ok, will change in next version.
> > > + break;
> > > + case PHY_INTERFACE_MODE_RGMII:
> > > + case PHY_INTERFACE_MODE_RGMII_TXID:
> > > + case PHY_INTERFACE_MODE_RGMII_RXID:
> > > + case PHY_INTERFACE_MODE_RGMII_ID:
> > > + intf_val |= PHY_INTF_RGMII;
> > > + break;
> > > + default:
> > > + dev_err(plat->dev, "phy interface not supported\n");
> > > + return -EINVAL;
> > > + }
> > > +
> > > + regmap_write(plat->peri_regmap, PERI_ETH_PHY_INTF_SEL, intf_val);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int mt2712_set_delay(struct mediatek_dwmac_plat_data *plat)
> > > +{
> > > + struct mac_delay_struct *mac_delay = &plat->mac_delay;
> > > + u32 delay_val = 0;
> > > + u32 fine_val = 0;
> > The same type declaration can be put into the one line
> >
> Got it.
> > > +
> > > + switch (plat->phy_mode) {
> >
> > There exists some room for code optimization in the switch statement
> > such as PHY_INTERFACE_MODE_MII and PHY_INTERFACE_MODE_RGMII both are
> > almost the same and even the configuration for the other PHY modes can
> > reuse their partial setup. It appears to be better using a common way
> > to set up various PHY modes.
> >
> I'll try define a function to handle it.
> And how about like this:
> static u32 delay_setting(u32 delay, bool inv, u32 en_bit, u32
> clk_shift, u32 clk_mask, u32 inv_bit) {
> u32 value = 0
>
> value |= delay ? en_bit : 0;
> value |= (delay << clk_shift) & clk_mask;
> value |= inv ? inv_bit : 0;
> }
>
> case PHY_INTERFACE_MODE_MII:
> delay_value |= delay_setting(mac_delay->tx_delay,
> mac_delay->tx_inv,
> PHY_DLY_TXC_ENABLE,
> PHY_DLY_TXC_SHIFT,
> PHY_DLY_TXC_STAGES,
> PHY_DLY_TXC_INV);
We can reuse FIELD_PREP defined in include/linux/bitfield.h to make up
of the value instead of creating your own function delay_setting here,
and also PHY_DLY_TXC_SHIFT macro can be trimmed while you're using
FIED_PREP
> delay_value |= delay_setting(mac_delay->rx_delay,
> mac_delay->rx_inv,
> PHY_DLY_RXC_ENABLE,
> PHY_DLY_RXC_SHIFT,
> PHY_DLY_RXC_STAGES,
> PHY_DLY_RXC_INV);
>
> case PHY_INTERFACE_MODE_RMII:
> if (plat->rmii_rxc) {
> delay_value |= delay_setting(mac_delay->rx_delay,
> mac_delay->rx_inv,
> PHY_DLY_RXC_ENABLE,
> PHY_DLY_RXC_SHIFT,
> PHY_DLY_RXC_STAGES,
> PHY_DLY_RXC_INV);
> fine_val |= mac_delay->tx_inv ?
> ETH_RMII_DLY_TX_INV : 0;
> } else {
> delay_value |= delay_setting(mac_delay->rx_delay,
> mac_delay->rx_inv,
shoudn't the parametors be mac_delay->tx_delay and mac_delay->tx_inv?
> PHY_DLY_TXC_ENABLE,
> PHY_DLY_TXC_SHIFT,
> PHY_DLY_TXC_STAGES,
> PHY_DLY_TXC_INV);
> fine_val |= mac_delay->tx_inv ?
> ETH_RMII_DLY_TX_INV : 0;
if (plat->tx_inv)
fine_val = ETH_RMII_DLY_TX_INV;
the default fine_val is zero so zero assignement can be trimmed when
!plat-> tx_inv
> }
> case PHY_INTERFACE_MODE_RGMII:
> fine_val = plat->fine_tune ?
> (ETH_FINE_DLY_GTXC | ETH_FINE_DLY_RXC) : 0;
if (plat->fine_tune)
fine_val = ETH_FINE_DLY_GTXC | ETH_FINE_DLY_RXC;
the default fine_val is zero so zero assignement can be trimmed when
!plat->fine_tune
> delay_value |= delay_setting(mac_delay->tx_delay,
> mac_delay->tx_inv,
> PHY_DLY_GTXC_ENABLE,
> PHY_DLY_GTXC_SHIFT,
> PHY_DLY_GTXC_STAGES,
> PHY_DLY_GTXC_INV);
> delay_value |= delay_setting(mac_delay->rx_delay,
> mac_delay->rx_inv,
> PHY_DLY_RXC_ENABLE,
> PHY_DLY_RXC_SHIFT,
> PHY_DLY_RXC_STAGES,
> PHY_DLY_RXC_INV);
> case PHY_INTERFACE_MODE_RGMII_TXID:
> fine_val = plat->fine_tune ? ETH_FINE_DLY_RXC : 0;
> delay_value |= delay_setting(mac_delay->rx_delay,
> mac_delay->rx_inv,
> PHY_DLY_RXC_ENABLE,
> PHY_DLY_RXC_SHIFT,
> PHY_DLY_RXC_STAGES,
> PHY_DLY_RXC_INV);
> case PHY_INTERFACE_MODE_RGMII_RXID:
> fine_val = plat->fine_tune ? ETH_FINE_DLY_GTXC : 0;
> delay_value |= delay_setting(mac_delay->tx_delay,
> mac_delay->tx_inv,
> PHY_DLY_GTXC_ENABLE,
> PHY_DLY_GTXC_SHIFT,
> PHY_DLY_GTXC_STAGES,
> PHY_DLY_GTXC_INV);
>
phy_mode is used to indicate what phy mode would be tweaked when mac
is connected to the phy so I thought mac delay can be independent from
phy internal delay that means PHY_INTERFACE_MODE_RGMII_RXID and
PHY_INTERFACE_MODE_RGMII_TXID can apply the same setting as
PHY_INTERFACE_MODE_RGMII does.
> > > + case PHY_INTERFACE_MODE_MII:
> > > + delay_val |= mac_delay->tx_delay ? PHY_DLY_TXC_ENABLE : 0;
> > > + delay_val |= (mac_delay->tx_delay << PHY_DLY_TXC_SHIFT) &
> > > + PHY_DLY_TXC_STAGES;
> > > + delay_val |= mac_delay->tx_inv ? PHY_DLY_TXC_INV : 0;
> > > + delay_val |= mac_delay->rx_delay ? PHY_DLY_RXC_ENABLE : 0;
> > > + delay_val |= (mac_delay->rx_delay << PHY_DLY_RXC_SHIFT) &
> > > + PHY_DLY_RXC_STAGES;
> > > + delay_val |= mac_delay->rx_inv ? PHY_DLY_RXC_INV : 0;
> > > + break;
> > > + case PHY_INTERFACE_MODE_RMII:
> > > + if (plat->rmii_rxc) {
> > > + delay_val |= mac_delay->rx_delay ?
> > > + PHY_DLY_RXC_ENABLE : 0;
> > > + delay_val |= (mac_delay->rx_delay <<
> > > + PHY_DLY_RXC_SHIFT) & PHY_DLY_RXC_STAGES;
> > > + delay_val |= mac_delay->rx_inv ? PHY_DLY_RXC_INV : 0;
> > > + fine_val |= mac_delay->tx_inv ?
> > > + ETH_RMII_DLY_TX_INV : 0;
> > why is fine_val got from tx_inv?
> >
> becase the tx inv will inverse the tx clock inside mac relative to
> reference clock from external phy, and this bit is located in the same
> register with fine-tune.
> maybe I should rename fine_val to avoid misunderstanding.
If you add more comments to say that, fine_val remains would be okay
> > > + } else {
> > > + delay_val |= mac_delay->rx_delay ?
> > > + PHY_DLY_TXC_ENABLE : 0;
> > > + delay_val |= (mac_delay->rx_delay <<
> > > + PHY_DLY_TXC_SHIFT) & PHY_DLY_TXC_STAGES;
> > > + delay_val |= mac_delay->rx_inv ? PHY_DLY_TXC_INV : 0;
> > > + fine_val |= mac_delay->tx_inv ?
> > > + ETH_RMII_DLY_TX_INV : 0;
> > ditto, why is fine_val got from tx_inv?
> >
> same as above. ETH_RMII_DLY_TX_INV is only for RMII, and located in the
> same register with fine-tune.
adding a fewer comments helps to avoid some confusion
> > > + }
> > > + break;
> > > + case PHY_INTERFACE_MODE_RGMII:
> > > + fine_val = plat->fine_tune ?
> > > + (ETH_FINE_DLY_GTXC | ETH_FINE_DLY_RXC) : 0;
> > > + delay_val |= mac_delay->tx_delay ? PHY_DLY_GTXC_ENABLE : 0;
> > > + delay_val |= mac_delay->tx_delay & PHY_DLY_GTXC_STAGES;
> > > + delay_val |= mac_delay->tx_inv ? PHY_DLY_GTXC_INV : 0;
> > > + delay_val |= mac_delay->rx_delay ? PHY_DLY_RXC_ENABLE : 0;
> > > + delay_val |= (mac_delay->rx_delay << PHY_DLY_RXC_SHIFT) &
> > > + PHY_DLY_RXC_STAGES;
> > > + delay_val |= mac_delay->rx_inv ? PHY_DLY_RXC_INV : 0;
> > > + break;
> > > + case PHY_INTERFACE_MODE_RGMII_TXID:
> > > + fine_val = plat->fine_tune ? ETH_FINE_DLY_RXC : 0;
> > > + delay_val |= mac_delay->rx_delay ? PHY_DLY_RXC_ENABLE : 0;
> > > + delay_val |= (mac_delay->rx_delay << PHY_DLY_RXC_SHIFT) &
> > > + PHY_DLY_RXC_STAGES;
> > > + delay_val |= mac_delay->rx_inv ? PHY_DLY_RXC_INV : 0;
> > why is PHY_INTERFACE_MODE_RGMII_TXID applied with *_RXC_* register
> > bits, not with *_TXC_* bits? I'm a little confused about what path the
> > register PHY_DLY_RXC_* cause the effects to? MAC to PHY or PHY to MAC?
> >
> The PHY_INTERFACE_MODE_RGMII_TXID is defined in
> Documentation/networking/phy.txt
> means phy will handle delay in tx path, so mac need handle delay in rx
> path here.
See the above explains: phy_mode is used to indicate what phy mode
would be tweaked when mac is connected to the phy so I thought mac
delay can be independent of phy internal delay that means
PHY_INTERFACE_MODE_RGMII_RXID and PHY_INTERFACE_MODE_RGMII_TXID can
apply the same setting as PHY_INTERFACE_MODE_RGMII does.
> > > + break;
> > > + case PHY_INTERFACE_MODE_RGMII_RXID:
> > > + fine_val = plat->fine_tune ? ETH_FINE_DLY_GTXC : 0;
> > > + delay_val |= mac_delay->tx_delay ? PHY_DLY_GTXC_ENABLE : 0;
> > > + delay_val |= mac_delay->tx_delay & PHY_DLY_GTXC_STAGES;
> > > + delay_val |= mac_delay->tx_inv ? PHY_DLY_GTXC_INV : 0;
> > ditto, as the above quetion
> >
> Similar answer as above.
> > > + break;
> > > + case PHY_INTERFACE_MODE_RGMII_ID:
> > > + break;
> > > + default:
> > > + dev_err(plat->dev, "phy interface not supported\n");
> > > + return -EINVAL;
> > > + }
> > > + regmap_write(plat->peri_regmap, PERI_ETH_PHY_DLY, delay_val);
> > > + regmap_write(plat->peri_regmap, PERI_ETH_DLY_FINE, fine_val);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static const struct mediatek_dwmac_variant mt2712_gmac_variant = {
> > > + .dwmac_set_phy_interface = mt2712_set_interface,
> > > + .dwmac_set_delay = mt2712_set_delay,
> > > + .clk_list = mt2712_dwmac_clk_l,
> > > + .num_clks = ARRAY_SIZE(mt2712_dwmac_clk_l),
> > > + .dma_bit_mask = 33,
> > > + .rx_delay_max = 32,
> > > + .tx_delay_max = 32,
> > > +};
> > > +
> > > +static int mediatek_dwmac_config_dt(struct mediatek_dwmac_plat_data *plat)
> > > +{
> > > + u32 tx_delay, rx_delay;
> > > +
> > > + plat->peri_regmap = syscon_regmap_lookup_by_phandle(plat->np, "mediatek,pericfg");
you're also missing the property definition in dt-binding.
> > > + if (IS_ERR(plat->peri_regmap)) {
> > > + dev_err(plat->dev, "Failed to get pericfg syscon\n");
> > > + return PTR_ERR(plat->peri_regmap);
> > > + }
> > > +
> > > + plat->phy_mode = of_get_phy_mode(plat->np);
> > > + if (plat->phy_mode < 0) {
> > > + dev_err(plat->dev, "not find phy-mode\n");
> > > + return -EINVAL;
> > > + }
> > > +
> > > + if (!of_property_read_u32(plat->np, "mediatek,tx-delay", &tx_delay)) {
> > > + if (tx_delay < plat->variant->tx_delay_max) {
> > > + plat->mac_delay.tx_delay = tx_delay;
> > > + } else {
> > > + dev_err(plat->dev, "Invalid TX clock delay: %d\n", tx_delay);
> > > + return -EINVAL;
> > > + }
> > > + }
> > > +
> > > + if (!of_property_read_u32(plat->np, "mediatek,rx-delay", &rx_delay)) {
> > > + if (rx_delay < plat->variant->rx_delay_max) {
> > > + plat->mac_delay.rx_delay = rx_delay;
> > > + } else {
> > > + dev_err(plat->dev, "Invalid RX clock delay: %d\n", rx_delay);
> > > + return -EINVAL;
> > > + }
> > > + }
> > > +
> > > + plat->mac_delay.tx_inv = of_property_read_bool(plat->np, "mediatek,txc-inverse");
> > > + plat->mac_delay.rx_inv = of_property_read_bool(plat->np, "mediatek,rxc-inverse");
> > > + plat->fine_tune = of_property_read_bool(plat->np, "mediatek,fine-tune");
> > > + plat->rmii_rxc = of_property_read_bool(plat->np, "mediatek,rmii-rxc");
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int mediatek_dwmac_clk_init(struct mediatek_dwmac_plat_data *plat)
> > > +{
> > > + const struct mediatek_dwmac_variant *variant = plat->variant;
> > > + int num = variant->num_clks;
> > > + int i;
> > put into the same line seems good
> >
> ok
> > > +
> > > + plat->clks = devm_kcalloc(plat->dev, num, sizeof(*plat->clks), GFP_KERNEL);
> > > + if (!plat->clks)
> > > + return -ENOMEM;
> > > +
> > > + for (i = 0; i < num; i++)
> > > + plat->clks[i].id = variant->clk_list[i];
> > > +
> > > + return devm_clk_bulk_get(plat->dev, num, plat->clks);
> > > +}
> > > +
> > > +static int mediatek_dwmac_init(struct platform_device *pdev, void *priv)
> > > +{
> > > + struct mediatek_dwmac_plat_data *plat = priv;
> > > + const struct mediatek_dwmac_variant *variant = plat->variant;
> > > + int ret = 0;
> > zero initialized seems unnecessary
> >
> ok, will not initialized here
> > > +
> > > + ret = dma_set_mask_and_coherent(plat->dev, DMA_BIT_MASK(variant->dma_bit_mask));
> > > + if (ret) {
> > > + dev_err(plat->dev, "No suitable DMA available, err = %d\n", ret);
> > > + return ret;
> > > + }
> > > +
> > > + ret = variant->dwmac_set_phy_interface(plat);
> > > + if (ret) {
> > > + dev_err(plat->dev, "failed to set phy interface, err = %d\n", ret);
> > > + return ret;
> > > + }
> > > +
> > > + ret = variant->dwmac_set_delay(plat);
> > > + if (ret) {
> > > + dev_err(plat->dev, "failed to set delay value, err = %d\n", ret);
> > > + return ret;
> > > + }
> > > +
> > > + ret = clk_bulk_prepare_enable(variant->num_clks, plat->clks);
> > > + if (ret) {
> > > + dev_err(plat->dev, "failed to enable clks, err = %d\n", ret);
> > > + return ret;
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static void mediatek_dwmac_exit(struct platform_device *pdev, void *priv)
> > > +{
> > > + struct mediatek_dwmac_plat_data *plat = priv;
> > > + const struct mediatek_dwmac_variant *variant = plat->variant;
> > > +
> > > + clk_bulk_disable_unprepare(variant->num_clks, plat->clks);
> > > +}
> > > +
> > > +static int mediatek_dwmac_probe(struct platform_device *pdev)
> > > +{
> > > + struct mediatek_dwmac_plat_data *priv_plat;
> > > + struct plat_stmmacenet_data *plat_dat;
> > > + struct stmmac_resources stmmac_res;
> > > + int ret = 0;
> > zero initialized seems unnecessary
> >
> ok, will not initialized here
> > > +
> > > + priv_plat = devm_kzalloc(&pdev->dev, sizeof(*priv_plat), GFP_KERNEL);
> > > + if (!priv_plat)
> > > + return -ENOMEM;
> > > +
> > > + priv_plat->variant = of_device_get_match_data(&pdev->dev);
> > > + if (!priv_plat->variant) {
> > > + dev_err(&pdev->dev, "Missing dwmac-mediatek variant\n");
> > > + return -EINVAL;
> > > + }
> > > +
> > > + priv_plat->dev = &pdev->dev;
> > > + priv_plat->np = pdev->dev.of_node;
> > > +
> > > + ret = mediatek_dwmac_config_dt(priv_plat);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + ret = mediatek_dwmac_clk_init(priv_plat);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + ret = stmmac_get_platform_resources(pdev, &stmmac_res);
> > > + if (ret)
> > > + return ret;
> > > +
< ... >
^ permalink raw reply
* Re: [PATCH net 0/2] ibmvnic: Fix queue and buffer accounting errors
From: David Miller @ 2018-11-22 19:53 UTC (permalink / raw)
To: tlfalcon; +Cc: netdev, linuxppc-dev, mwb, julietk, tyreld
In-Reply-To: <1542820679-30184-1-git-send-email-tlfalcon@linux.ibm.com>
From: Thomas Falcon <tlfalcon@linux.ibm.com>
Date: Wed, 21 Nov 2018 11:17:57 -0600
> This series includes two small fixes. The first resolves a typo bug
> in the code to clean up unused RX buffers during device queue removal.
> The second ensures that device queue memory is updated to reflect new
> supported queue ring sizes after migration to other backing hardware.
Series applied.
^ permalink raw reply
* Re: Issue with RTL8111 NIC after upgrade to kernel 4.19
From: Heiner Kallweit @ 2018-11-22 19:53 UTC (permalink / raw)
To: Marc Dionne
Cc: andrew, norbert.jurkeit, nic_swsd, Florian Fainelli, David Miller,
netdev, Linux Kernel Mailing List, michael.wiktowy, jcline
In-Reply-To: <CAB9dFdvscM_vjhautiNc6yNZ4_aMQOkcGeKmAyRv4E191q9jaQ@mail.gmail.com>
On 22.11.2018 20:29, Marc Dionne wrote:
> On Thu, Nov 22, 2018 at 2:17 PM Heiner Kallweit <hkallweit1@gmail.com> wrote:
>> Thanks a lot for testing. Could you please test also the following
>> as an alternative to the delay?
>>
>> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
>> index 55202a0ac..aeccb2323 100644
>> --- a/drivers/net/phy/phy_device.c
>> +++ b/drivers/net/phy/phy_device.c
>> @@ -2254,6 +2254,7 @@ int phy_driver_register(struct phy_driver *new_driver, struct module *owner)
>> new_driver->mdiodrv.driver.probe = phy_probe;
>> new_driver->mdiodrv.driver.remove = phy_remove;
>> new_driver->mdiodrv.driver.owner = owner;
>> + new_driver->mdiodrv.driver.probe_type = PROBE_FORCE_SYNCHRONOUS;
>>
>> retval = driver_register(&new_driver->mdiodrv.driver);
>> if (retval) {
>
> That also gets me network reliably, switching between a kernel where
> it fails (distro 4.19 kernel) and the custom kernel with the patch.
>
Thanks again for the very quick response. The result is good and bad
news at once: good because it seems we have a fix, bad because we
don't understand the root cause of the issue yet.
>>> There's a side issue that network startup is taking a full minute
>>> longer than it should, but that's possibly unrelated.
>
> BTW that's an unrelated rng issue, for which I have a workaround.
>
Indeed totally unrelated, but sounds familiar. I had the same issue
on a headless system which therefore generates very little entropy.
I fixed it by setting CONFIG_RANDOM_TRUST_CPU=y
>>>
>> Thanks, Heiner
>
> Marc
>
Heiner
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox