Netdev List
 help / color / mirror / Atom feed
* [PATCH net-next] net: mscc: ocelot: remove set but not used variable 'phy_mode'
From: YueHaibing @ 2018-10-08 14:07 UTC (permalink / raw)
  To: Alexandre Belloni, David S. Miller; +Cc: YueHaibing, netdev, kernel-janitors

Fixes gcc '-Wunused-but-set-variable' warning:

drivers/net/ethernet/mscc/ocelot_board.c: In function 'mscc_ocelot_probe':
drivers/net/ethernet/mscc/ocelot_board.c:262:17: warning:
 variable 'phy_mode' set but not used [-Wunused-but-set-variable]
   enum phy_mode phy_mode;

It never used since introduction in 
commit 71e32a20cfbf ("net: mscc: ocelot: make use of SerDes PHYs for handling their configuration")

Signed-off-by: YueHaibing <yuehaibing@huawei.com>
---
 drivers/net/ethernet/mscc/ocelot_board.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/net/ethernet/mscc/ocelot_board.c b/drivers/net/ethernet/mscc/ocelot_board.c
index 953b326..0cf0b09 100644
--- a/drivers/net/ethernet/mscc/ocelot_board.c
+++ b/drivers/net/ethernet/mscc/ocelot_board.c
@@ -259,7 +259,6 @@ static int mscc_ocelot_probe(struct platform_device *pdev)
 		struct phy_device *phy;
 		struct resource *res;
 		struct phy *serdes;
-		enum phy_mode phy_mode;
 		void __iomem *regs;
 		char res_name[8];
 		u32 port;
@@ -297,10 +296,8 @@ static int mscc_ocelot_probe(struct platform_device *pdev)
 		case PHY_INTERFACE_MODE_NA:
 			continue;
 		case PHY_INTERFACE_MODE_SGMII:
-			phy_mode = PHY_MODE_SGMII;
 			break;
 		case PHY_INTERFACE_MODE_QSGMII:
-			phy_mode = PHY_MODE_QSGMII;
 			break;
 		default:
 			dev_err(ocelot->dev,

^ permalink raw reply related

* Re: [PATCH 1/9] PCI: sysfs: Export available PCIe bandwidth
From: Alex_Gagniuc @ 2018-10-08 21:09 UTC (permalink / raw)
  To: helgaas
  Cc: mr.nuke.me, linux-pci, bhelgaas, keith.busch, Austin.Bolen,
	Shyam.Iyer, ariel.elior, everest-linux-l2, davem, michael.chan,
	ganeshgr, jeffrey.t.kirsher, tariqt, saeedm, leon, jakub.kicinski,
	dirk.vandermerwe, netdev, linux-kernel, intel-wired-lan,
	linux-rdma, oss-drivers, stephen, mj, alex.williamson
In-Reply-To: <20181004204549.GI120535@bhelgaas-glaptop.roam.corp.google.com>

On 10/04/2018 03:45 PM, Bjorn Helgaas wrote:
> 
> [EXTERNAL EMAIL]
> Please report any suspicious attachments, links, or requests for sensitive information.
> 
> 
> [+cc Alex (VC mentioned below)]
> 
> On Wed, Oct 03, 2018 at 10:00:19PM +0000, Alex_Gagniuc@Dellteam.com wrote:
>> On 10/03/2018 04:31 PM, Bjorn Helgaas wrote:
>>> On Mon, Sep 03, 2018 at 01:02:28PM -0500, Alexandru Gagniuc wrote:
>>>> For certain bandwidth-critical devices (e.g. multi-port network cards)
>>>> it is useful to know the available bandwidth to the root complex. This
>>>> information is only available via the system log, which doesn't
>>>> account for link degradation after probing.
>>>>
>>>> With a sysfs attribute, we can computes the bandwidth on-demand, and
>>>> will detect degraded links.
>>>>
>>>> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
>>>> ---
>>>>    drivers/pci/pci-sysfs.c | 13 +++++++++++++
>>>>    1 file changed, 13 insertions(+)
>>>>
>>>> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
>>>> index 9ecfe13157c0..6658e927b1f5 100644
>>>> --- a/drivers/pci/pci-sysfs.c
>>>> +++ b/drivers/pci/pci-sysfs.c
>>>> @@ -218,6 +218,18 @@ static ssize_t current_link_width_show(struct device *dev,
>>>>    }
>>>>    static DEVICE_ATTR_RO(current_link_width);
>>>>    
>>>> +static ssize_t available_bandwidth_show(struct device *dev,
>>>> +				       struct device_attribute *attr, char *buf)
>>>> +{
>>>> +	struct pci_dev *pci_dev = to_pci_dev(dev);
>>>> +	u32 bw_avail;
>>>> +
>>>> +	bw_avail = pcie_bandwidth_available(pci_dev, NULL, NULL, NULL);
>>>> +
>>>> +	return sprintf(buf, "%u.%03u Gb/s\n", bw_avail / 1000, bw_avail % 1000);
>>>> +}
>>>> +static DEVICE_ATTR_RO(available_bandwidth);
>>>
>>> Help me understand this.  We already have these sysfs attributes:
>>>
>>>     max_link_speed          # eg, 16 GT/s
>>>     max_link_width          # eg, 8
>>>     current_link_speed      # eg, 16 GT/s
>>>     current_link_width      # eg, 8
>>>
>>> so I think the raw materials are already exposed.
>>>
>>> The benefits I see for this new file are that
>>>
>>>     - pcie_bandwidth_available() does the work of traversing up the
>>>       tree, doing the computations (link width * speed, reduced by
>>>       encoding overhead), and finding the minimum, and
>>>
>>>     - it re-traverses the path every time we look at it, while the
>>>       boot-time check is a one-time event.
>>>
>>> In principle this could all be done in user space with the attributes
>>> that are already exported.  There's some precedent for things like
>>> this in lspci, e.g., "NUMA node" [1], and lspci might even be a more
>>> user-friendly place for users to look for this, as opposed to
>>> searching through sysfs.
>>
>> Parsing the endpoint to root port bandwidth is, in principle, possible
>> from userspace. It's just that in practice it's very clumsy to do, and,
>> as you pointed out, not that reliable.
> 
> I don't remember the reliability issue.  Was that in a previous
> conversation?  AFAICT, using current_link_speed and current_link_width
> should be as reliable as pcie_bandwidth_available() because they're
> both based on reading PCI_EXP_LNKSTA.
> 
> This patch exposes only the available bandwidth, not the limiting
> device, link speed, or link width.  Maybe that extra information isn't
> important in this context.  Of course, it's easy to derive using
> current_link_speed and current_link_width, but if we do that, there's
> really no benefit to adding a new file.
> 
> Linux doesn't really support Virtual Channels (VC) yet, and I don't
> know much about it, but it seems like Quality-of-Service features like
> VC could affect this idea of available bandwidth.
> 
> Since we already expose the necessary information, and we might throw
> additional things like VC into the mix, I'm a little hesitant about
> adding things to sysfs because they're very difficult to change later.

I understand your concerns with VC and crazy PCIe outliers. 
'available_bandwidth' is meant to mean physical bandwidth, rather than 
"what comcast gives you". I see now the possibility for confusion.
My motivation is to save drivers from the hassle of having to call 
pcie_print_link_status(), and prevent the rare duplicate syslog messages.

I prefer re-using the kernel logic over doing it over again from 
userspace, but I won't insist over your doubts.

Alex

>> I understand it's not information that all users would jump in the air
>> to know. However, it was important enough for certain use cases, that
>> the kernel already has a very reliable way to calculate it.
>>
>> It seems to me that the most elegant way is to let the kernel tell us,
>> since the kernel already has this facility. To quote one of the texts
>> under Documentation/, it is an elegant way to "avoid reinventing kernel
>> wheels in userspace".
>>
>> Alex
>>
>>> [1] https://git.kernel.org/pub/scm/utils/pciutils/pciutils.git/commit/?id=90ec4a6d0ae8
>>>
>>>>    static ssize_t secondary_bus_number_show(struct device *dev,
>>>>    					 struct device_attribute *attr,
>>>>    					 char *buf)
>>>> @@ -786,6 +798,7 @@ static struct attribute *pcie_dev_attrs[] = {
>>>>    	&dev_attr_current_link_width.attr,
>>>>    	&dev_attr_max_link_width.attr,
>>>>    	&dev_attr_max_link_speed.attr,
>>>> +	&dev_attr_available_bandwidth.attr,
>>>>    	NULL,
>>>>    };
>>>>    
>>>> -- 
>>>> 2.17.1
>>>>
>>>
>>
> 


^ permalink raw reply

* Re: [PATCH v8 08/15] octeontx2-af: Add RVU block LF provisioning support
From: Sunil Kovvuri @ 2018-10-08 13:59 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Linux Netdev List, David S. Miller, linux-soc, Sunil Goutham
In-Reply-To: <CAK8P3a3koXRBzLmuaBOD2GmdV8v6AdKDNr+tGCTQCz5yjzQ7wQ@mail.gmail.com>

On Mon, Oct 8, 2018 at 5:41 PM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Sun, Oct 7, 2018 at 5:00 PM <sunil.kovvuri@gmail.com> wrote:
> >
> > +/* Structure for requesting resource provisioning.
> > + * 'modify' flag to be used when either requesting more
> > + * or to detach partial of a cetain resource type.
> > + * Rest of the fields specify how many of what type to
> > + * be attached.
> > + */
> > +struct rsrc_attach {
> > +       struct mbox_msghdr hdr;
> > +       u8   modify:1;
> > +       u8   npalf:1;
> > +       u8   nixlf:1;
> > +       u16  sso;
> > +       u16  ssow;
> > +       u16  timlfs;
> > +       u16  cptlfs;
> > +};
> > +
> > +/* Structure for relinquishing resources.
> > + * 'partial' flag to be used when relinquishing all resources
> > + * but only of a certain type. If not set, all resources of all
> > + * types provisioned to the RVU function will be detached.
> > + */
> > +struct rsrc_detach {
> > +       struct mbox_msghdr hdr;
> > +       u8 partial:1;
> > +       u8 npalf:1;
> > +       u8 nixlf:1;
> > +       u8 sso:1;
> > +       u8 ssow:1;
> > +       u8 timlfs:1;
> > +       u8 cptlfs:1;
> > +};
>
> Are these bitfields part of the message that gets sent to the
> underlying implementation? It seems there is still an endianess
> issue then.
>
>        Arnd


No these structures are not used for kernel driver to firmware
communication where
register reads via readq are involved. These structures are used for
mailbox communication
between different PCI devices and this mailbox is a shared memory.

Sunil.

^ permalink raw reply

* Re: [PATCH net-next 05/19] net: usb: aqc111: Introduce PHY access
From: Oliver Neukum @ 2018-10-08 13:52 UTC (permalink / raw)
  To: Igor Russkikh, David S . Miller
  Cc: Dmitry Bezrukov, linux-usb@vger.kernel.org,
	netdev@vger.kernel.org
In-Reply-To: <6b4837b970d7709bc2e11d89a7e21e5f10584e30.1538734658.git.igor.russkikh@aquantia.com>

On Fr, 2018-10-05 at 10:24 +0000, Igor Russkikh wrote:
> From: Dmitry Bezrukov <dmitry.bezrukov@aquantia.com>
> 
> Implement PHY power up/down sequences.
> AQC111, depending on FW used, may has PHY being controlled either
> directly (dpa = 1) or via vendor command interface (dpa = 0).
> Drivers supports both themes.
> We determine this from firmware versioning agreement.
> 
> Signed-off-by: Dmitry Bezrukov <dmitry.bezrukov@aquantia.com>
> Signed-off-by: Igor Russkikh <igor.russkikh@aquantia.com>
> ---
>  drivers/net/usb/aqc111.c | 93 ++++++++++++++++++++++++++++++++++++++++++++++++
>  drivers/net/usb/aqc111.h | 70 ++++++++++++++++++++++++++++++++++++
>  2 files changed, 163 insertions(+)
> 
> diff --git a/drivers/net/usb/aqc111.c b/drivers/net/usb/aqc111.c
> index 22bb259d71fb..30219bb6ddfd 100644
> --- a/drivers/net/usb/aqc111.c
> +++ b/drivers/net/usb/aqc111.c
> @@ -138,15 +138,44 @@ static int aqc111_write_cmd(struct usbnet *dev, u8 cmd, u16 value,
>  	return __aqc111_write_cmd(dev, cmd, value, index, size, data, 0);
>  }
>  
> +static int aq_mdio_read_cmd(struct usbnet *dev, u16 value, u16 index,
> +			    u16 size, void *data)
> +{
> +	return aqc111_read_cmd(dev, AQ_PHY_CMD, value, index, size, data);
> +}
> +
> +static int aq_mdio_write_cmd(struct usbnet *dev, u16 value, u16 index,
> +			     u16 size, void *data)
> +{
> +	return aqc111_write_cmd(dev, AQ_PHY_CMD, value, index, size, data);
> +}
> +
>  static const struct net_device_ops aqc111_netdev_ops = {
>  	.ndo_open		= usbnet_open,
>  	.ndo_stop		= usbnet_stop,
>  };
>  
> +static void aqc111_read_fw_version(struct usbnet *dev,
> +				   struct aqc111_data *aqc111_data)
> +{
> +	aqc111_read_cmd(dev, AQ_ACCESS_MAC, AQ_FW_VER_MAJOR,
> +			1, 1, &aqc111_data->fw_ver.major);
> +	aqc111_read_cmd(dev, AQ_ACCESS_MAC, AQ_FW_VER_MINOR,
> +			1, 1, &aqc111_data->fw_ver.minor);
> +	aqc111_read_cmd(dev, AQ_ACCESS_MAC, AQ_FW_VER_REV,
> +			1, 1, &aqc111_data->fw_ver.rev);

Why read the stuff you don't need?

> +
> +	if (aqc111_data->fw_ver.major & 0x80)
> +		aqc111_data->fw_ver.major &= ~0x80;
> +	else
> +		aqc111_data->dpa = 1;
> +}
> +
>  static int aqc111_bind(struct usbnet *dev, struct usb_interface *intf)
>  {
>  	int ret;
>  	struct usb_device *udev = interface_to_usbdev(intf);
> +	struct aqc111_data *aqc111_data;
>  
>  	/* Check if vendor configuration */
>  	if (udev->actconfig->desc.bConfigurationValue != 1) {
> @@ -162,8 +191,18 @@ static int aqc111_bind(struct usbnet *dev, struct usb_interface *intf)
>  		return ret;
>  	}
>  
> +	aqc111_data = kzalloc(sizeof(*aqc111_data), GFP_KERNEL);
> +	if (!aqc111_data)
> +		return -ENOMEM;
> +
> +	/* store aqc111_data pointer in device data field */
> +	dev->data[0] = (unsigned long)aqc111_data;
> +	memset(aqc111_data, 0, sizeof(*aqc111_data));

Either kzalloc() or a memset. Not both.
> +
>  	dev->net->netdev_ops = &aqc111_netdev_ops;
>  
> +	aqc111_read_fw_version(dev, aqc111_data);
> +
>  	return 0;
>  }

	Regards
		Oliver

^ permalink raw reply

* Re: [PATCH] bpf: btf: Fix a missing check bug
From: valdis.kletnieks @ 2018-10-08 21:17 UTC (permalink / raw)
  To: Song Liu
  Cc: wang6495, kjlu, Alexei Starovoitov, Daniel Borkmann, Networking,
	open list
In-Reply-To: <CAPhsuW5vnVjRS7DWFYPSmq5G-dAFxOrHCAOMkK-iaOoirzMazQ@mail.gmail.com>

[-- Attachment #1: Type: text/plain, Size: 1069 bytes --]

On Mon, 08 Oct 2018 13:51:09 -0700, Song Liu said:
> On Sun, Oct 7, 2018 at 1:26 PM Wenwen Wang <wang6495@umn.edu> wrote:

> > same value. Given that the btf data is in the user space, a malicious user
> > can race to change the data between the two copies. By doing so, the user
> > can provide malicious data to the kernel and cause undefined behavior.

> These two numbers are copied from same memory location, right? So I
> think this check is not necessary?

Security researchers call this a TOCTOU bug - Time of Check - Time of Use.

What can happen:

1) We fetch the value  (say we get 90) from userspace and stash it in hdr_len.

2) We do some other stuff like check the hdr_len isn't too big, etc..

        meanwhile, on another CPU running another thread of the process...
                3) malicious code stuffs a 117 into that field

4) We fetch the entire header, incliding a now-changed hdr_len  (now 117) and
stick it in btf->hdr->hdr_len.

5) Any code that assumes that hdr_len and btf->hdr->hdr_len are the same value
explodes in interesting ways.



[-- Attachment #2: Type: application/pgp-signature, Size: 486 bytes --]

^ permalink raw reply

* Re: [PATCH net-next 06/19] net: usb: aqc111: Introduce link management
From: Oliver Neukum @ 2018-10-08 13:59 UTC (permalink / raw)
  To: Igor Russkikh, David S . Miller
  Cc: Dmitry Bezrukov, linux-usb@vger.kernel.org,
	netdev@vger.kernel.org
In-Reply-To: <cec5cd88988e0985e3fdd1343906ef649b01f646.1538734658.git.igor.russkikh@aquantia.com>

On Fr, 2018-10-05 at 10:24 +0000, Igor Russkikh wrote:
> From: Dmitry Bezrukov <dmitry.bezrukov@aquantia.com>
> 
> +static void aqc111_configure_rx(struct usbnet *dev,
> +				struct aqc111_data *aqc111_data)
> +{
> +	u8 reg8 = 0;
> +	u8 queue_num = 0;
> +	u16 reg16 = 0;
> +	u16 link_speed = 0, usb_host = 0;
> +	u8 buf[5] = { 0 };

DMA on stack.

> +	enum usb_device_speed usb_speed = dev->udev->speed;
> +
> +	buf[0] = 0x00;
> +	buf[1] = 0xF8;
> +	buf[2] = 0x07;
> +	switch (aqc111_data->link_speed) {
> +	case AQ_INT_SPEED_5G:
> +	{
> +		link_speed = 5000;
> +		reg8 = 0x05;
> +		reg16 = 0x001F;
> +		break;
> +	}
> +	case AQ_INT_SPEED_2_5G:
> +	{
> +		link_speed = 2500;
> +		reg16 = 0x003F;
> +		break;
> +	}
> +	case AQ_INT_SPEED_1G:
> +	{
> +		link_speed = 1000;
> +		reg16 = 0x009F;
> +		break;
> +	}
> +	case AQ_INT_SPEED_100M:
> +	{
> +		link_speed = 100;
> +		queue_num = 1;
> +		reg16 = 0x063F;
> +		buf[1] = 0xFB;
> +		buf[2] = 0x4;
> +		break;
> +	}
> +	}
> +
> +	if (aqc111_data->dpa) {
> +		/* Set Phy Flow control */
> +		aq_mdio_write_cmd(dev, AQ_GLB_ING_PAUSE_CTRL_REG,
> +				  AQ_PHY_AUTONEG_ADDR, 2, &reg16);
> +		aq_mdio_write_cmd(dev, AQ_GLB_EGR_PAUSE_CTRL_REG,
> +				  AQ_PHY_AUTONEG_ADDR, 2, &reg16);
> +	}
> +
> +	aqc111_write_cmd(dev, AQ_ACCESS_MAC, SFR_INTER_PACKET_GAP_0,
> +			 1, 1, &reg8);
> +
> +	aqc111_write_cmd(dev, AQ_ACCESS_MAC, SFR_TX_PAUSE_RESEND_T, 3, 3, buf);
> +
> +	switch (usb_speed) {

Needs to handle all speeds. And please add a comment explaining the
rationale.

> +	case USB_SPEED_SUPER:
> +	{
> +		usb_host = 3;
> +		break;
> +	}
> +	case USB_SPEED_HIGH:
> +	{
> +		usb_host = 2;
> +		break;
> +	}
> +	case USB_SPEED_FULL:
> +	case USB_SPEED_LOW:
> +	{
> +		usb_host = 1;
> +		queue_num = 0;
> +		break;
> +	}
> +	default:
> +	{
> +		usb_host = 0;
> +		break;
> +	}
> +	}
> +
> +	memcpy(buf, &AQC111_BULKIN_SIZE[queue_num], 5);
> +	/* RX bulk configuration */
> +	aqc111_write_cmd(dev, AQ_ACCESS_MAC, SFR_RX_BULKIN_QCTRL, 5, 5, buf);
> +
> +	/* Set high low water level */
> +	reg16 = 0x0810;
> +
> +	aqc111_write_cmd(dev, AQ_ACCESS_MAC, SFR_PAUSE_WATERLVL_LOW,
> +			 2, 2, &reg16);
> +	netdev_info(dev->net, "Link Speed %d, USB %d", link_speed, usb_host);
> +}

	Regards
		Oliver

^ permalink raw reply

* Re: [PATCH net-next 19/19] net: usb: aqc111: Add support for wake on LAN by MAGIC packet
From: Igor Russkikh @ 2018-10-08 14:12 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: David S . Miller, linux-usb@vger.kernel.org,
	netdev@vger.kernel.org, Dmitry Bezrukov
In-Reply-To: <20181006174921.GG6990@lunn.ch>

Hi Andrew,

>> +		if (aqc111_data->dpa) {
>> +			aqc111_set_phy_speed(dev, AUTONEG_DISABLE, SPEED_100);
> 
> I don't think that works. You should leave AUTONEG on, but only
> advertise SPEED_100 and trigger auto-neg. If you force it to 100,
> there is no guarantee the peer will figure out what the new link speed
> is. I've often seen failed auto-net result in 10/Half. So you will
> loose the link, making WoL pointless.

Phy does not support 10M, low power mode explicitly uses 100M
for power safety reasons.

It is meaningless here to add Autoneg to 100M because thats the only
speedmask bit anyway.


>> +	aqc111_set_phy_speed(dev, aqc111_data->autoneg,
>> +			     aqc111_data->advertised_speed);
>> +
> 
> Should that be conditional on aqc111_data->dpa?

Actually no, because set_phy_speed internally checks this flag.

>> +	u8 rsvd[283];
>> +};
> 
> Do you really need these 283 bytes??

>>  	struct aqc111_phy_options phy_ops;
>> +	struct aqc111_wol_cfg wol_cfg;
> 
> Those 283 bytes make this whole structure bigger...

FW interface expects the WOL config request WOL_CFG_SIZE bytes.
These reserved fields are just not used now by linux driver.
They configure extra wol features like a sleep proxy.
Thus, we anyway have to allocate this somewhere.

Regards,
  Igor

^ permalink raw reply

* [PATCH net-next] dpaa2-eth: Don't account Tx confirmation frames on NAPI poll
From: Ioana Ciocoi Radulescu @ 2018-10-08 14:16 UTC (permalink / raw)
  To: netdev@vger.kernel.org, davem@davemloft.net; +Cc: Ioana Ciornei

Until now, both Rx and Tx confirmation frames handled during
NAPI poll were counted toward the NAPI budget. However, Tx
confirmations are lighter to process than Rx frames, which can
skew the amount of work actually done inside one NAPI cycle.

Update the code to only count Rx frames toward the NAPI budget
and set a separate threshold on how many Tx conf frames can be
processed in one poll cycle.

The NAPI poll routine stops when either the budget is consumed
by Rx frames or when Tx confirmation frames reach this threshold.

Signed-off-by: Ioana Radulescu <ruxandra.radulescu@nxp.com>
---
 drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c | 68 +++++++++++++++---------
 drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.h |  5 ++
 2 files changed, 47 insertions(+), 26 deletions(-)

diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c
index 108c137..156080d 100644
--- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c
+++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c
@@ -289,10 +289,11 @@ static void dpaa2_eth_rx(struct dpaa2_eth_priv *priv,
  *
  * Observance of NAPI budget is not our concern, leaving that to the caller.
  */
-static int consume_frames(struct dpaa2_eth_channel *ch)
+static int consume_frames(struct dpaa2_eth_channel *ch,
+			  enum dpaa2_eth_fq_type *type)
 {
 	struct dpaa2_eth_priv *priv = ch->priv;
-	struct dpaa2_eth_fq *fq;
+	struct dpaa2_eth_fq *fq = NULL;
 	struct dpaa2_dq *dq;
 	const struct dpaa2_fd *fd;
 	int cleaned = 0;
@@ -311,12 +312,23 @@ static int consume_frames(struct dpaa2_eth_channel *ch)
 
 		fd = dpaa2_dq_fd(dq);
 		fq = (struct dpaa2_eth_fq *)(uintptr_t)dpaa2_dq_fqd_ctx(dq);
-		fq->stats.frames++;
 
 		fq->consume(priv, ch, fd, &ch->napi, fq->flowid);
 		cleaned++;
 	} while (!is_last);
 
+	if (!cleaned)
+		return 0;
+
+	fq->stats.frames += cleaned;
+	ch->stats.frames += cleaned;
+
+	/* A dequeue operation only pulls frames from a single queue
+	 * into the store. Return the frame queue type as an out param.
+	 */
+	if (type)
+		*type = fq->type;
+
 	return cleaned;
 }
 
@@ -921,14 +933,16 @@ static int pull_channel(struct dpaa2_eth_channel *ch)
 static int dpaa2_eth_poll(struct napi_struct *napi, int budget)
 {
 	struct dpaa2_eth_channel *ch;
-	int cleaned = 0, store_cleaned;
 	struct dpaa2_eth_priv *priv;
+	int rx_cleaned = 0, txconf_cleaned = 0;
+	enum dpaa2_eth_fq_type type;
+	int store_cleaned;
 	int err;
 
 	ch = container_of(napi, struct dpaa2_eth_channel, napi);
 	priv = ch->priv;
 
-	while (cleaned < budget) {
+	do {
 		err = pull_channel(ch);
 		if (unlikely(err))
 			break;
@@ -936,30 +950,32 @@ static int dpaa2_eth_poll(struct napi_struct *napi, int budget)
 		/* Refill pool if appropriate */
 		refill_pool(priv, ch, priv->bpid);
 
-		store_cleaned = consume_frames(ch);
-		cleaned += store_cleaned;
+		store_cleaned = consume_frames(ch, &type);
+		if (type == DPAA2_RX_FQ)
+			rx_cleaned += store_cleaned;
+		else
+			txconf_cleaned += store_cleaned;
 
-		/* If we have enough budget left for a full store,
-		 * try a new pull dequeue, otherwise we're done here
+		/* If we either consumed the whole NAPI budget with Rx frames
+		 * or we reached the Tx confirmations threshold, we're done.
 		 */
-		if (store_cleaned == 0 ||
-		    cleaned > budget - DPAA2_ETH_STORE_SIZE)
-			break;
-	}
-
-	if (cleaned < budget && napi_complete_done(napi, cleaned)) {
-		/* Re-enable data available notifications */
-		do {
-			err = dpaa2_io_service_rearm(ch->dpio, &ch->nctx);
-			cpu_relax();
-		} while (err == -EBUSY);
-		WARN_ONCE(err, "CDAN notifications rearm failed on core %d",
-			  ch->nctx.desired_cpu);
-	}
+		if (rx_cleaned >= budget ||
+		    txconf_cleaned >= DPAA2_ETH_TXCONF_PER_NAPI)
+			return budget;
+	} while (store_cleaned);
 
-	ch->stats.frames += cleaned;
+	/* We didn't consume the entire budget, so finish napi and
+	 * re-enable data availability notifications
+	 */
+	napi_complete_done(napi, rx_cleaned);
+	do {
+		err = dpaa2_io_service_rearm(ch->dpio, &ch->nctx);
+		cpu_relax();
+	} while (err == -EBUSY);
+	WARN_ONCE(err, "CDAN notifications rearm failed on core %d",
+		  ch->nctx.desired_cpu);
 
-	return cleaned;
+	return max(rx_cleaned, 1);
 }
 
 static void enable_ch_napi(struct dpaa2_eth_priv *priv)
@@ -1076,7 +1092,7 @@ static u32 drain_channel(struct dpaa2_eth_priv *priv,
 
 	do {
 		pull_channel(ch);
-		drained = consume_frames(ch);
+		drained = consume_frames(ch, NULL);
 		total += drained;
 	} while (drained);
 
diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.h b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.h
index 7a7a3e7..452a8e9 100644
--- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.h
+++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.h
@@ -40,6 +40,11 @@
  */
 #define DPAA2_ETH_TAILDROP_THRESH	(64 * 1024)
 
+/* Maximum number of Tx confirmation frames to be processed
+ * in a single NAPI call
+ */
+#define DPAA2_ETH_TXCONF_PER_NAPI	256
+
 /* Buffer quota per queue. Must be large enough such that for minimum sized
  * frames taildrop kicks in before the bpool gets depleted, so we compute
  * how many 64B frames fit inside the taildrop threshold and add a margin
-- 
2.7.4

^ permalink raw reply related

* Re: [PATCH net-next 08/19] net: usb: aqc111: Implement TX data path
From: Oliver Neukum @ 2018-10-08 14:07 UTC (permalink / raw)
  To: Igor Russkikh, Andrew Lunn
  Cc: Dmitry Bezrukov, David S . Miller, linux-usb@vger.kernel.org,
	netdev@vger.kernel.org
In-Reply-To: <3d55fcaf-3297-f315-b258-2887d5a64cf6@aquantia.com>

On Mo, 2018-10-08 at 13:43 +0000, Igor Russkikh wrote:
> > > +	skb_push(skb, AQ_TX_HEADER_SIZE);
> > > +	cpu_to_le64s(&tx_hdr);
> > 
> > Is that portable? tx_hdr is a structure of 2x u32 bitfields.  What
> > endian have you tested that one?
> > 
> 
> You are right, this is wrong for BE hardware.
> 
> We don't have such a hardware to check unfortunately.
> Think its better to drop endianess conversions and declare
> the driver as little endian only.
> 
> Do you think that'll be acceptable?

No. If worse comes to worse define it u64 and set the values
manually.

	Regards
		Oliver

^ permalink raw reply

* [PATCH] qed: fix memory leak of pent on error exit paths
From: Colin King @ 2018-10-08 14:17 UTC (permalink / raw)
  To: Ariel Elior, everest-linux-l2, David S . Miller; +Cc: kernel-janitors, netdev

From: Colin Ian King <colin.king@canonical.com>

Currently, calls to qed_sp_init_request can leak pent on -EINVAL errors.
Fix this by adding the allocated pent back to the p_hwfn free_pool
on these error exits so it can be re-used later and hence fixes the
leak.

Detected by CoverityScan, CID#1411643 ("Resource leak")

Fixes: fe56b9e6a8d9 ("qed: Add module with basic common support")
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
 .../net/ethernet/qlogic/qed/qed_sp_commands.c    | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/qlogic/qed/qed_sp_commands.c b/drivers/net/ethernet/qlogic/qed/qed_sp_commands.c
index 77b6248ad3b9..810475a6522a 100644
--- a/drivers/net/ethernet/qlogic/qed/qed_sp_commands.c
+++ b/drivers/net/ethernet/qlogic/qed/qed_sp_commands.c
@@ -79,8 +79,10 @@ int qed_sp_init_request(struct qed_hwfn *p_hwfn,
 		break;
 
 	case QED_SPQ_MODE_BLOCK:
-		if (!p_data->p_comp_data)
-			return -EINVAL;
+		if (!p_data->p_comp_data) {
+			rc = -EINVAL;
+			goto err;
+		}
 
 		p_ent->comp_cb.cookie = p_data->p_comp_data->cookie;
 		break;
@@ -95,7 +97,8 @@ int qed_sp_init_request(struct qed_hwfn *p_hwfn,
 	default:
 		DP_NOTICE(p_hwfn, "Unknown SPQE completion mode %d\n",
 			  p_ent->comp_mode);
-		return -EINVAL;
+		rc = -EINVAL;
+		goto err;
 	}
 
 	DP_VERBOSE(p_hwfn, QED_MSG_SPQ,
@@ -109,6 +112,13 @@ int qed_sp_init_request(struct qed_hwfn *p_hwfn,
 	memset(&p_ent->ramrod, 0, sizeof(p_ent->ramrod));
 
 	return 0;
+
+err:
+	qed_spq_return_entry(p_hwfn, *pp_ent);
+	*pp_ent = NULL;
+
+	return rc;
+
 }
 
 static enum tunnel_clss qed_tunn_clss_to_fw_clss(u8 type)
-- 
2.17.1

^ permalink raw reply related

* Re: [PATCH net-next 05/19] net: usb: aqc111: Introduce PHY access
From: Igor Russkikh @ 2018-10-08 14:07 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: David S . Miller, linux-usb@vger.kernel.org,
	netdev@vger.kernel.org, Dmitry Bezrukov
In-Reply-To: <20181008121707.GB6216@lunn.ch>


Hi Andrew,

>>>> +	struct aqc111_data *aqc111_data = (struct aqc111_data *)dev->data[0];
>>>
>>> Having to do this cast all the time is quiet ugly. It seems like some
>>> other usb_net drivers use netdev_priv().
>>
>> As I see most of usb usbnet based devices use the same theme with accessing
>> private data via dev->data.
> 
> It is just ugly. It would of been better if dev->data[] was a void
> pointer. This is the first usbnet driver i've reviewed, so i don't
> know the history behind this. I wonder if adding a void *priv would be
> accepted?

I can't comment on history of this, but...

net-next/drivers/net/usb$ grep "dev->data" * | wc -l
173

Regards,
  Igor

^ permalink raw reply

* Re: [PATCH net-next 05/19] net: usb: aqc111: Introduce PHY access
From: Igor Russkikh @ 2018-10-08 14:10 UTC (permalink / raw)
  To: Oliver Neukum, David S . Miller
  Cc: Dmitry Bezrukov, linux-usb@vger.kernel.org,
	netdev@vger.kernel.org
In-Reply-To: <1539006774.10342.16.camel@suse.com>

Hi Oliver,

>> +	aqc111_read_cmd(dev, AQ_ACCESS_MAC, AQ_FW_VER_MAJOR,
>> +			1, 1, &aqc111_data->fw_ver.major);
>> +	aqc111_read_cmd(dev, AQ_ACCESS_MAC, AQ_FW_VER_MINOR,
>> +			1, 1, &aqc111_data->fw_ver.minor);
>> +	aqc111_read_cmd(dev, AQ_ACCESS_MAC, AQ_FW_VER_REV,
>> +			1, 1, &aqc111_data->fw_ver.rev);
> 
> Why read the stuff you don't need?

fw_ver is used below to determine phy access mode.

fw_ver.rev is not used in this exact patch, thats true,
but it gets reported in later patches in the set.

Regards,
  Igor

^ permalink raw reply

* Re: [PATCH net-next 13/19] net: usb: aqc111: Add support for TSO
From: Oliver Neukum @ 2018-10-08 14:12 UTC (permalink / raw)
  To: Igor Russkikh, David S . Miller
  Cc: Dmitry Bezrukov, linux-usb@vger.kernel.org,
	netdev@vger.kernel.org
In-Reply-To: <b4e69e6a2b05929bbba681f1bdd115eb32cb64b0.1538734658.git.igor.russkikh@aquantia.com>

On Fr, 2018-10-05 at 10:25 +0000, Igor Russkikh wrote:
> From: Dmitry Bezrukov <dmitry.bezrukov@aquantia.com>
> 
> Signed-off-by: Dmitry Bezrukov <dmitry.bezrukov@aquantia.com>
> Signed-off-by: Igor Russkikh <igor.russkikh@aquantia.com>
> ---
>  drivers/net/usb/aqc111.c | 3 +++
>  drivers/net/usb/aqc111.h | 6 ++++--
>  2 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/usb/aqc111.c b/drivers/net/usb/aqc111.c
> index 6efd9a9ad44e..f61fa7446b72 100644
> --- a/drivers/net/usb/aqc111.c
> +++ b/drivers/net/usb/aqc111.c
> @@ -964,6 +964,9 @@ static struct sk_buff *aqc111_tx_fixup(struct usbnet *dev, struct sk_buff *skb,
>  	/*Length of actual data*/
>  	tx_hdr.length = (skb->len & 0x1FFFFF);
>  
> +	/* TSO MSS */
> +	tx_hdr.max_seg_size = skb_shinfo(skb)->gso_size;

Endianness
> +
>  	headroom = (skb->len + AQ_TX_HEADER_SIZE) % 8;
>  	if (headroom != 0)
>  		padding_size = 8 - headroom;

	Regards
		Oliver

^ permalink raw reply

* Re: [PATCH net-next 15/19] net: usb: aqc111: Add support for VLAN_CTAG_TX/RX offload
From: Oliver Neukum @ 2018-10-08 14:14 UTC (permalink / raw)
  To: Igor Russkikh, David S . Miller
  Cc: Dmitry Bezrukov, linux-usb@vger.kernel.org,
	netdev@vger.kernel.org
In-Reply-To: <7d3c26e57edd4cc4f0995e66ec71be9d1c8f53d9.1538734658.git.igor.russkikh@aquantia.com>

On Fr, 2018-10-05 at 10:25 +0000, Igor Russkikh wrote:
> From: Dmitry Bezrukov <dmitry.bezrukov@aquantia.com>
> 
> Signed-off-by: Dmitry Bezrukov <dmitry.bezrukov@aquantia.com>
> Signed-off-by: Igor Russkikh <igor.russkikh@aquantia.com>
> ---
>  drivers/net/usb/aqc111.c | 14 ++++++++++++++
>  drivers/net/usb/aqc111.h |  7 ++++++-
>  2 files changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/usb/aqc111.c b/drivers/net/usb/aqc111.c
> index cc23c39beab3..a9051dd7c5bd 100644
> --- a/drivers/net/usb/aqc111.c
> +++ b/drivers/net/usb/aqc111.c
> @@ -524,6 +524,7 @@ static int aqc111_bind(struct usbnet *dev, struct usb_interface *intf)
>  
>  	dev->net->hw_features |= AQ_SUPPORT_HW_FEATURE;
>  	dev->net->features |= AQ_SUPPORT_FEATURE;
> +	dev->net->vlan_features |= AQ_SUPPORT_VLAN_FEATURE;
>  
>  	aqc111_read_fw_version(dev, aqc111_data);
>  	aqc111_data->autoneg = AUTONEG_ENABLE;
> @@ -817,6 +818,7 @@ static int aqc111_reset(struct usbnet *dev)
>  
>  	dev->net->hw_features |= AQ_SUPPORT_HW_FEATURE;
>  	dev->net->features |= AQ_SUPPORT_FEATURE;
> +	dev->net->vlan_features |= AQ_SUPPORT_VLAN_FEATURE;
>  
>  	/* Power up ethernet PHY */
>  	aqc111_data->phy_ops.advertising = 0;
> @@ -992,6 +994,11 @@ static int aqc111_rx_fixup(struct usbnet *dev, struct sk_buff *skb)
>  		new_skb->truesize = new_skb->len + sizeof(struct sk_buff);
>  		if (aqc111_data->rx_checksum)
>  			aqc111_rx_checksum(new_skb, &pkt_desc);
> +		if (pkt_desc->vlan_ind)
> +			__vlan_hwaccel_put_tag(new_skb,
> +					       htons(ETH_P_8021Q),
> +					       pkt_desc->vlan_tag &
> +					       VLAN_VID_MASK);
>  
>  		usbnet_skb_return(dev, new_skb);
>  		if (pkt_count == 0)
> @@ -1020,6 +1027,7 @@ static struct sk_buff *aqc111_tx_fixup(struct usbnet *dev, struct sk_buff *skb,
>  	int tailroom = 0;
>  	int padding_size = 0;
>  	struct sk_buff *new_skb = NULL;
> +	u16 tci = 0;
>  
>  	memset(&tx_hdr, 0x00, sizeof(tx_hdr));
>  
> @@ -1038,6 +1046,12 @@ static struct sk_buff *aqc111_tx_fixup(struct usbnet *dev, struct sk_buff *skb,
>  		tx_hdr.drop_padding = 1;
>  	}
>  
> +	/* Vlan Tag */
> +	if (vlan_get_tag(skb, &tci) >= 0) {
> +		tx_hdr.vlan_tag = 1;
> +		tx_hdr.vlan_info = tci;

Endianness

	Regards
		Oliver

^ permalink raw reply

* Re: [PATCH net-next 18/19] net: usb: aqc111: Implement get/set_link_ksettings callbacks
From: Oliver Neukum @ 2018-10-08 14:18 UTC (permalink / raw)
  To: Igor Russkikh, David S . Miller
  Cc: Dmitry Bezrukov, linux-usb@vger.kernel.org,
	netdev@vger.kernel.org
In-Reply-To: <694b148fa9e79d42fe8a815821ae5b0bc12c4aea.1538734658.git.igor.russkikh@aquantia.com>

On Fr, 2018-10-05 at 10:25 +0000, Igor Russkikh wrote:
> From: Dmitry Bezrukov <dmitry.bezrukov@aquantia.com>
> 
> +static int aqc111_get_link_ksettings(struct net_device *net,
> +				     struct ethtool_link_ksettings *elk)
> +{
> +	struct usbnet *dev = netdev_priv(net);
> +	enum usb_device_speed usb_speed = dev->udev->speed;
> +	struct aqc111_data *aqc111_data = (struct aqc111_data *)dev->data[0];
> +	u32 speed = SPEED_UNKNOWN;
> +
> +	ethtool_link_ksettings_zero_link_mode(elk, supported);
> +	ethtool_link_ksettings_add_link_mode(elk, supported,
> +					     100baseT_Full);
> +	ethtool_link_ksettings_add_link_mode(elk, supported,
> +					     1000baseT_Full);
> +	if (usb_speed == USB_SPEED_SUPER) {

And SUPER_PLUS?

	Regards
		Oliver

^ permalink raw reply

* Re: [PATCH v8 01/15] octeontx2-af: Add Marvell OcteonTX2 RVU AF driver
From: Arnd Bergmann @ 2018-10-08 14:29 UTC (permalink / raw)
  To: Sunil Kovvuri; +Cc: Networking, David Miller, linux-soc, sgoutham
In-Reply-To: <CA+sq2Cdi2VqoycE9Z37sToro=q-mrX5jqq+HyL_bFKn+-DHoyA@mail.gmail.com>

On Mon, Oct 8, 2018 at 3:50 PM Sunil Kovvuri <sunil.kovvuri@gmail.com> wrote:
>
> On Mon, Oct 8, 2018 at 5:52 PM Arnd Bergmann <arnd@arndb.de> wrote:
> >
> > On Sun, Oct 7, 2018 at 4:59 PM <sunil.kovvuri@gmail.com> wrote:
> >
> > > --- /dev/null
> > > +++ b/drivers/net/ethernet/marvell/octeontx2/Kconfig
> > > @@ -0,0 +1,12 @@
> > > +#
> > > +# Marvell OcteonTX2 drivers configuration
> > > +#
> > > +
> > > +config OCTEONTX2_AF
> > > +       tristate "Marvell OcteonTX2 RVU Admin Function driver"
> > > +       depends on ARM64 && PCI
> >
> > You should try to allow building it on x86 and other architectures, even though
> > the driver won't be used there this helps get reports from static
> > build infrastructure.
> > You could use e.g.
> >
> >       depends on (64BIT && COMPILE_TEST) || ARM64
> >       depends on PCI
> >
> >            Arnd
>
> Thanks for the suggestion.
> But going forward we will have few arm64 assembly instructions used in
> the driver.

What is those inline assembly for? Is there a chance they can
be abstracted through some other interface?

     Arnd

^ permalink raw reply

* Re: [PATCH v8 08/15] octeontx2-af: Add RVU block LF provisioning support
From: Arnd Bergmann @ 2018-10-08 14:29 UTC (permalink / raw)
  To: Sunil Kovvuri; +Cc: Networking, David Miller, linux-soc, sgoutham
In-Reply-To: <CA+sq2CcXCEY14d+dsFJvokFLykweeZUxPp911-oVyvW=Ye1N=A@mail.gmail.com>

On Mon, Oct 8, 2018 at 3:59 PM Sunil Kovvuri <sunil.kovvuri@gmail.com> wrote:
> On Mon, Oct 8, 2018 at 5:41 PM Arnd Bergmann <arnd@arndb.de> wrote:
> > On Sun, Oct 7, 2018 at 5:00 PM <sunil.kovvuri@gmail.com> wrote:
> > >
> > > +/* Structure for requesting resource provisioning.
> > > + * 'modify' flag to be used when either requesting more
> > > + * or to detach partial of a cetain resource type.
> > > + * Rest of the fields specify how many of what type to
> > > + * be attached.
> > > + */
> > > +struct rsrc_attach {
> > > +       struct mbox_msghdr hdr;
> > > +       u8   modify:1;
> > > +       u8   npalf:1;
> > > +       u8   nixlf:1;
> > > +       u16  sso;
> > > +       u16  ssow;
> > > +       u16  timlfs;
> > > +       u16  cptlfs;
> > > +};
> > > +
> > > +/* Structure for relinquishing resources.
> > > + * 'partial' flag to be used when relinquishing all resources
> > > + * but only of a certain type. If not set, all resources of all
> > > + * types provisioned to the RVU function will be detached.
> > > + */
> > > +struct rsrc_detach {
> > > +       struct mbox_msghdr hdr;
> > > +       u8 partial:1;
> > > +       u8 npalf:1;
> > > +       u8 nixlf:1;
> > > +       u8 sso:1;
> > > +       u8 ssow:1;
> > > +       u8 timlfs:1;
> > > +       u8 cptlfs:1;
> > > +};
> >
> > Are these bitfields part of the message that gets sent to the
> > underlying implementation? It seems there is still an endianess
> > issue then.
>
> No these structures are not used for kernel driver to firmware
> communication where
> register reads via readq are involved. These structures are used for
> mailbox communication
> between different PCI devices and this mailbox is a shared memory.

Ok, thanks for the clarification.

       Arnd

^ permalink raw reply

* Re: [PATCH net-next 00/19] Add support for Aquantia AQtion USB to 5/2.5GbE devices
From: Oliver Neukum @ 2018-10-08 14:21 UTC (permalink / raw)
  To: Igor Russkikh, David S . Miller
  Cc: linux-usb@vger.kernel.org, netdev@vger.kernel.org
In-Reply-To: <cover.1538734658.git.igor.russkikh@aquantia.com>

On Fr, 2018-10-05 at 10:24 +0000, Igor Russkikh wrote:
> This patchset introduces support for new multigig ethernet to USB dongle,
> developed jointly by Aquantia (Phy) and ASIX (USB MAC).
> 
> The driver has similar structure with other ASIX MAC drivers (AX88179), but
> with a number of important differences:
> - Driver supports both direct Phy and custom firmware interface for Phy
>   programming. This is due to different firmware modules available with this
>   product.
> - Driver handles new 2.5G/5G link speed configuration and reporting.
> - Device support all speeds from 100M up to 5G.
> - Device supports MTU up to 16K.
> 
> Device supports various standard networking features, like
> checksum offloads, vlan tagging/filtering, TSO.
> 
> The code of this driver is based on original ASIX sources and was extended
> by Aquantia for 5G multigig support. 

Thank you for the driver. It is good to see drivers for cool hardware.
Unfortunately there have been a few issues I have tried to point out
in reviews. Please fix them and resubmit.

	Regards
		Oliver

^ permalink raw reply

* Re: [PATCH net-next 05/19] net: usb: aqc111: Introduce PHY access
From: Oliver Neukum @ 2018-10-08 14:24 UTC (permalink / raw)
  To: Igor Russkikh, David S . Miller
  Cc: Dmitry Bezrukov, linux-usb@vger.kernel.org,
	netdev@vger.kernel.org
In-Reply-To: <747f70d8-303a-aeea-2359-26b03a5e0336@aquantia.com>

On Mo, 2018-10-08 at 17:10 +0300, Igor Russkikh wrote:
> Hi Oliver,
> 
> > > +	aqc111_read_cmd(dev, AQ_ACCESS_MAC, AQ_FW_VER_MAJOR,
> > > +			1, 1, &aqc111_data->fw_ver.major);
> > > +	aqc111_read_cmd(dev, AQ_ACCESS_MAC, AQ_FW_VER_MINOR,
> > > +			1, 1, &aqc111_data->fw_ver.minor);
> > > +	aqc111_read_cmd(dev, AQ_ACCESS_MAC, AQ_FW_VER_REV,
> > > +			1, 1, &aqc111_data->fw_ver.rev);
> > 
> > Why read the stuff you don't need?
> 
> fw_ver is used below to determine phy access mode.
> 
> fw_ver.rev is not used in this exact patch, thats true,
> but it gets reported in later patches in the set.

Hi,

OK that makes sense.

	Regards
		Oliver

^ permalink raw reply

* Re: [PATCH V1 net 0/5] minor bug fixes for ENA Ethernet driver
From: Machulsky, Zorik @ 2018-10-08 14:38 UTC (permalink / raw)
  To: Bshara, Nafea, Kiyanovski, Arthur, davem@davemloft.net,
	netdev@vger.kernel.org
  Cc: Woodhouse, David, Matushevsky, Alexander, Bshara, Saeed,
	Wilson, Matt, Liguori, Anthony, Tzalik, Guy, Belgazal, Netanel,
	Saidi, Ali
In-Reply-To: <7BF68A50-AD04-4FA7-91ED-F3F6412E2E2E@amazon.com>

Ship it

On 10/8/18, 5:47 AM, "Bshara, Nafea" <nafea@amazon.com> wrote:

    Ship it
    
    On 10/8/18, 5:28 AM, "akiyano@amazon.com" <akiyano@amazon.com> wrote:
    
        From: Arthur Kiyanovski <akiyano@amazon.com>
        
        Arthur Kiyanovski (5):
          net: ena: fix indentations in ena_defs for better readability
          net: ena: fix warning in rmmod caused by double iounmap
          net: ena: fix rare bug when failed restart/resume is followed by
            driver removal
          net: ena: fix NULL dereference due to untimely napi initialization
          net: ena: fix auto casting to boolean
        
         drivers/net/ethernet/amazon/ena/ena_admin_defs.h  | 308 +++++++++-------------
         drivers/net/ethernet/amazon/ena/ena_eth_com.c     |   8 +-
         drivers/net/ethernet/amazon/ena/ena_eth_io_defs.h | 219 ++++++++-------
         drivers/net/ethernet/amazon/ena/ena_netdev.c      |  23 +-
         drivers/net/ethernet/amazon/ena/ena_regs_defs.h   | 206 +++++++--------
         5 files changed, 341 insertions(+), 423 deletions(-)
        
        -- 
        2.7.4
        
        
    
    


^ permalink raw reply

* Re: [PATCH V1 net 0/5] minor bug fixes for ENA Ethernet driver
From: Machulsky, Zorik @ 2018-10-08 14:42 UTC (permalink / raw)
  To: Bshara, Nafea, Kiyanovski, Arthur, davem@davemloft.net,
	netdev@vger.kernel.org
  Cc: Woodhouse, David, Matushevsky, Alexander, Bshara, Saeed,
	Wilson, Matt, Liguori, Anthony, Tzalik, Guy, Belgazal, Netanel,
	Saidi, Ali
In-Reply-To: <7BF68A50-AD04-4FA7-91ED-F3F6412E2E2E@amazon.com>

Acked-by: Zorik Machulsky <zorik@amazon.com>
    
    On 10/8/18, 5:28 AM, "akiyano@amazon.com" <akiyano@amazon.com> wrote:
    
        From: Arthur Kiyanovski <akiyano@amazon.com>
        
        Arthur Kiyanovski (5):
          net: ena: fix indentations in ena_defs for better readability
          net: ena: fix warning in rmmod caused by double iounmap
          net: ena: fix rare bug when failed restart/resume is followed by
            driver removal
          net: ena: fix NULL dereference due to untimely napi initialization
          net: ena: fix auto casting to boolean
        
         drivers/net/ethernet/amazon/ena/ena_admin_defs.h  | 308 +++++++++-------------
         drivers/net/ethernet/amazon/ena/ena_eth_com.c     |   8 +-
         drivers/net/ethernet/amazon/ena/ena_eth_io_defs.h | 219 ++++++++-------
         drivers/net/ethernet/amazon/ena/ena_netdev.c      |  23 +-
         drivers/net/ethernet/amazon/ena/ena_regs_defs.h   | 206 +++++++--------
         5 files changed, 341 insertions(+), 423 deletions(-)
        
        -- 
        2.7.4
        
        
    
    


^ permalink raw reply

* Re: [PATCH net-next 19/19] net: usb: aqc111: Add support for wake on LAN by MAGIC packet
From: Andrew Lunn @ 2018-10-08 14:47 UTC (permalink / raw)
  To: Igor Russkikh
  Cc: David S . Miller, linux-usb@vger.kernel.org,
	netdev@vger.kernel.org, Dmitry Bezrukov
In-Reply-To: <0c6bc63e-2bc8-2479-ed22-ba174493478e@aquantia.com>

On Mon, Oct 08, 2018 at 02:12:59PM +0000, Igor Russkikh wrote:
> Hi Andrew,
> 
> >> +		if (aqc111_data->dpa) {
> >> +			aqc111_set_phy_speed(dev, AUTONEG_DISABLE, SPEED_100);
> > 
> > I don't think that works. You should leave AUTONEG on, but only
> > advertise SPEED_100 and trigger auto-neg. If you force it to 100,
> > there is no guarantee the peer will figure out what the new link speed
> > is. I've often seen failed auto-net result in 10/Half. So you will
> > loose the link, making WoL pointless.
> 
> Phy does not support 10M, low power mode explicitly uses 100M
> for power safety reasons.
> 
> It is meaningless here to add Autoneg to 100M because thats the only
> speedmask bit anyway.

If you have AUTONEG_DISABLE, i would assume you PHY is not even trying
to auto_neg. So the speedmask is irrelevent, it is not sent to the
peer. And since the peer is not receiving any auto-neg information, it
will fail to auto-neg, and most likely default to 10/Half.

To do this right, please take a look at this commit

commit 2b9672ddb6f347467d7b33b86c5dfc4d5c0501a8
Author: Heiner Kallweit <hkallweit1@gmail.com>
Date:   Thu Jul 12 21:32:53 2018 +0200

    net: phy: add phy_speed_down and phy_speed_up
    
    Some network drivers include functionality to speed down the PHY when
    suspending and just waiting for a WoL packet because this saves energy.
    This functionality is quite generic, therefore let's factor it out to
    phylib.
    
> 
> >> +	aqc111_set_phy_speed(dev, aqc111_data->autoneg,
> >> +			     aqc111_data->advertised_speed);
> >> +
> > 
> > Should that be conditional on aqc111_data->dpa?
> 
> Actually no, because set_phy_speed internally checks this flag.

So you should probably remove the check above when forcing the speed
to 100. Make the code symmetrical. 

> 
> >> +	u8 rsvd[283];
> >> +};
> > 
> > Do you really need these 283 bytes??
> 
> >>  	struct aqc111_phy_options phy_ops;
> >> +	struct aqc111_wol_cfg wol_cfg;
> > 
> > Those 283 bytes make this whole structure bigger...
> 
> FW interface expects the WOL config request WOL_CFG_SIZE bytes.
> These reserved fields are just not used now by linux driver.
> They configure extra wol features like a sleep proxy.
> Thus, we anyway have to allocate this somewhere.

Well, i think your low level function for actually sending a command
does a dup before sending. You don't actually send this, you send a
copy. Maybe you can pad it out then?

      Andrew

^ permalink raw reply

* Re: [PATCH net-next 00/19] Add support for Aquantia AQtion USB to 5/2.5GbE devices
From: Igor Russkikh @ 2018-10-08 14:52 UTC (permalink / raw)
  To: Oliver Neukum, David S . Miller
  Cc: linux-usb@vger.kernel.org, netdev@vger.kernel.org,
	Dmitry Bezrukov
In-Reply-To: <1539008496.10342.27.camel@suse.com>



On 08.10.2018 17:21, Oliver Neukum wrote:

>> The code of this driver is based on original ASIX sources and was extended
>> by Aquantia for 5G multigig support. 
> 
> Thank you for the driver. It is good to see drivers for cool hardware.
> Unfortunately there have been a few issues I have tried to point out
> in reviews. Please fix them and resubmit.
> 

Thank you Oliver for valuable input, working on this already.

Regards,
  Igor

^ permalink raw reply

* Re: selftests/bpf: test_kmod.sh hangs on all devices
From: Naresh Kamboju @ 2018-10-08 14:55 UTC (permalink / raw)
  To: Daniel Borkmann
  Cc: open list:KERNEL SELFTEST FRAMEWORK, Shuah Khan, Anders Roxell,
	Rafael Tinoco, ast, netdev, songliubraving, Willem de Bruijn
In-Reply-To: <febf5bc7-9e4e-1383-33bc-00416cc90e20@iogearbox.net>

Daniel,

On Mon, 8 Oct 2018 at 18:58, Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On 10/08/2018 03:13 PM, Naresh Kamboju wrote:
> > BPF test case test_kmod.sh hangs on all devices running linux next.
> >
> > + cd /opt/kselftests/default-in-kernel/bpf
> > + ./test_kmod.sh
> > sysctl: cannot stat /proc/sys/net/core/bpf_jit_enable: No such file or directory
> > sysctl: cannot stat /proc/sys/net/core/bpf_jit_harden: No such file or directory
> > sysctl: cannot stat /proc/sys/net/core/bpf_jit_enable: No such file or directory
> > sysctl: cannot stat /proc/sys/net/core/bpf_jit_harden: No such file or directory
> > [ JIT enabled:0 hardened:0 ]
> >
> > https://lkft.validation.linaro.org/scheduler/job/429726
> >
> > Test hangs started from 4.19.0-rc4-next-20180918.
> > Linux version 4.19.0-rc4-next-20180918 (oe-user@oe-host) (gcc version
> > 7.1.1 20170707 (Linaro GCC 7.1-2017.08)) #1 SMP Tue Sep 18 05:26:00
> > UTC 2018
> >
> > History can be compared from this page.
> > https://qa-reports.linaro.org/lkft/linux-next-oe/tests/kselftest/bpf_test_kmod.sh
> >
> > OTOH,
> > There is a kernel BUG,
>
> This is quite an old linux-next kernel, should be fixed by 100811936f89 ("bpf: test_bpf:
> add init_net to dev for flow_dissector"). Please make sure you have that commit included
> in your testing:

I will re-validate on latest code base and let you know.

>
> https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git/commit/?id=100811936f89fd455eda1984810c09003550555b

Thanks for the quick reply.

- Naresh
>
> Thanks,
> Daniel

^ permalink raw reply

* Re: [PATCH V2 net-next 2/5] net: Introduce a new MII time stamping interface.
From: Andrew Lunn @ 2018-10-08 15:07 UTC (permalink / raw)
  To: Richard Cochran
  Cc: netdev, devicetree, David Miller, Florian Fainelli, Jacob Keller,
	Mark Rutland, Miroslav Lichvar, Rob Herring, Willem de Bruijn
In-Reply-To: <20181008020439.m2cytnnv5ue7yc5l@localhost>

On Sun, Oct 07, 2018 at 07:04:39PM -0700, Richard Cochran wrote:
> On Sun, Oct 07, 2018 at 09:54:00PM +0200, Andrew Lunn wrote:
> > Sure, but things have moved on since then.
> 
> I was curious about this.  Based on your uses cases, I guess that you
> mean phylib?  But not much has changed AFAICT. (There is one new
> global function and two were removed, but that doesn't change the
> picture WRT time stamping.)
> 
> Phylink now has two or three new users, one of which is dsa.  Is that
> the big move?
> 
> The situation with MACs that handle their own PHYs without phylib is
> unchanged, AFAICT.
> 
> So what exactly do you mean?

Hi Richard

We are pushing phylink. I really do think anything using > 1Gbps links
should be using phylink, not phydev. And i think we have reached the
tipping point, that most new MACs will be > 1Gbps. 2.5G or maybe 5G
will be the new default. The MAC-PHY link is quiet messy when you get
above 1G. There are a number of options which you can use, and the MAC
and PHY need to negotiate a common set to use. phylink can do this,
phylib cannot easily do it. So i see phylib slowly becoming a legacy
API for MAC drivers.

We are also slowly seeing more SFPs, and Linux controlling them. SFP
are not new, they have been in top end switches for a long time. But
they are slowly becoming more popular in industrial settings, and such
embedded industrial systems tend to let Linux control them, not
firmware. And i think industry makes more use of PTP than other
fields, but i could be wrong. Since optical SFP modules are passive, a
bump-in-the-wire time stamper actually makes sense for these.

Also, fibre on the last mile is slowly becoming more of a thing, so
maybe we will start seeing CPE, consumer routers, with SFP ports?

As i said before, we are seeing more MACs which use firmware for
controlling the PHYs. I'm not sure why yet. Maybe it is coupled with
more MACs supporting > 1G, which is messy. Or the lack of good PHY
drivers for PHYs which support > 1G? Hopefully the drivers will
improve with time.

So as you said, the phylib API has not changed much, which is common
for mature code. But i think long term, it will become less important.
It will share the space with phylink. And any code which wants to be
generically usable, should not depend on phydev. Architecturally, it
seems wrong for you to hang what should be a generic time stamping
framework on phydev. It is not future proof. net_device is future
proof.

	  Andrew

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox