* Re: [PATCH RFC] Add Microchip KSZ8895 DSA driver
From: Pavel Machek @ 2017-10-11 21:03 UTC (permalink / raw)
To: Tristram.Ha
Cc: Andrew Lunn, Florian Fainelli, muvarov, nathan.leigh.conrad,
vivien.didelot, UNGLinuxDriver, netdev, linux-kernel
In-Reply-To: <1507322033-15222-2-git-send-email-Tristram.Ha@microchip.com>
[-- Attachment #1: Type: text/plain, Size: 2626 bytes --]
Hi!
> +static void ksz8895_set_prio_queue(struct ksz_device *dev, int port, int queue)
> +{
> + u8 hi;
> + u8 lo;
> +
> + /* Number of queues can only be 1, 2, or 4. */
> + switch (queue) {
> + case 4:
> + case 3:
> + queue = PORT_QUEUE_SPLIT_4;
> + break;
> + case 2:
> + queue = PORT_QUEUE_SPLIT_2;
> + break;
> + default:
> + queue = PORT_QUEUE_SPLIT_1;
> + }
> + ksz_pread8(dev, port, REG_PORT_CTRL_0, &lo);
> + ksz_pread8(dev, port, P_DROP_TAG_CTRL, &hi);
> + lo &= ~PORT_QUEUE_SPLIT_L;
> + if (queue & PORT_QUEUE_SPLIT_2)
> + lo |= PORT_QUEUE_SPLIT_L;
> + hi &= ~PORT_QUEUE_SPLIT_H;
> + if (queue & PORT_QUEUE_SPLIT_4)
> + hi |= PORT_QUEUE_SPLIT_H;
> + ksz_pwrite8(dev, port, REG_PORT_CTRL_0, lo);
> + ksz_pwrite8(dev, port, P_DROP_TAG_CTRL, hi);
> +
> + /* Default is port based for egress rate limit. */
> + if (queue != PORT_QUEUE_SPLIT_1)
> + ksz_cfg(dev, REG_SW_CTRL_19, SW_OUT_RATE_LIMIT_QUEUE_BASED,
> + true);
> +}
This is same as the other driver, right? Same comments apply here, and
please find a way to make it shared.
> +static void ksz8895_r_mib_cnt(struct ksz_device *dev, int port, u16 addr,
> + u64 *cnt)
> +{
> + u32 data;
> + u16 ctrl_addr;
> + u8 check;
> + int loop;
> +
> + ctrl_addr = addr + SWITCH_COUNTER_NUM * port;
> + ctrl_addr |= IND_ACC_TABLE(TABLE_MIB | TABLE_READ);
> +
> + mutex_lock(&dev->alu_mutex);
> + ksz_write16(dev, REG_IND_CTRL_0, ctrl_addr);
> +
> + /* It is almost guaranteed to always read the valid bit because of
> + * slow SPI speed.
> + */
> + for (loop = 2; loop > 0; loop--) {
> + ksz_read8(dev, REG_IND_MIB_CHECK, &check);
> +
> + if (check & MIB_COUNTER_VALID) {
> + ksz_read32(dev, REG_IND_DATA_LO, &data);
> + if (check & MIB_COUNTER_OVERFLOW)
> + *cnt += MIB_COUNTER_VALUE + 1;
> + *cnt += data & MIB_COUNTER_VALUE;
> + break;
> + }
> + }
> + mutex_unlock(&dev->alu_mutex);
Again, same function, same review comments.
> +static void ksz8895_r_table(struct ksz_device *dev, int table, u16 addr,
> + u64 *data)
> +{
> + u16 ctrl_addr;
> +
> + ctrl_addr = IND_ACC_TABLE(table | TABLE_READ) | addr;
> +
> + mutex_lock(&dev->alu_mutex);
> + ksz_write16(dev, REG_IND_CTRL_0, ctrl_addr);
> + ksz_get(dev, REG_IND_DATA_HI, data, sizeof(u64));
> + mutex_unlock(&dev->alu_mutex);
> + *data = be64_to_cpu(*data);
> +}
I've seen this before; this is duplicated code, it does not make sense
to review.
--
(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: [PATCH 3/4] dpaa_eth: change device used
From: David Miller @ 2017-10-11 21:02 UTC (permalink / raw)
To: madalin.bucur
Cc: netdev, f.fainelli, andrew, vivien.didelot, junote, linux-kernel
In-Reply-To: <1507644618-32006-4-git-send-email-madalin.bucur@nxp.com>
From: Madalin Bucur <madalin.bucur@nxp.com>
Date: Tue, 10 Oct 2017 17:10:17 +0300
> @@ -2696,7 +2681,13 @@ static int dpaa_eth_probe(struct platform_device *pdev)
> int err = 0, i, channel;
> struct device *dev;
>
> - dev = &pdev->dev;
> + /* device used for DMA mapping */
> + dev = pdev->dev.parent;
> + err = dma_coerce_mask_and_coherent(dev, DMA_BIT_MASK(40));
> + if (err) {
> + dev_err(dev, "dma_coerce_mask_and_coherent() failed\n");
> + goto dev_mask_failed;
> + }
>
> /* Allocate this early, so we can store relevant information in
> * the private area
Since you are moving this code up before the netdev allocation, you must
adjust the failure path goto label used.
Your change as-is will cause an OOPS because we'll pass a NULL pointer
to free_netdev().
^ permalink raw reply
* Re: [PATCH v1 RFC 1/1] Add Microchip KSZ8795 DSA driver
From: Pavel Machek @ 2017-10-11 20:59 UTC (permalink / raw)
To: Tristram.Ha
Cc: Andrew Lunn, Florian Fainelli, Ruediger Schmitt, muvarov,
nathan.leigh.conrad, vivien.didelot, UNGLinuxDriver, netdev,
linux-kernel
In-Reply-To: <1507322023-15182-2-git-send-email-Tristram.Ha@microchip.com>
[-- Attachment #1: Type: text/plain, Size: 3869 bytes --]
Hi!
> +static void ksz8795_set_prio_queue(struct ksz_device *dev, int port, int queue)
> +{
> + u8 hi;
> + u8 lo;
> +
> + /* Number of queues can only be 1, 2, or 4. */
> + switch (queue) {
> + case 4:
> + case 3:
> + queue = PORT_QUEUE_SPLIT_4;
> + break;
> + case 2:
> + queue = PORT_QUEUE_SPLIT_2;
> + break;
> + default:
> + queue = PORT_QUEUE_SPLIT_1;
> + }
If only 1, 2 and 4 are valid, it probably should not accept other
values?
> +static void ksz8795_r_mib_cnt(struct ksz_device *dev, int port, u16 addr,
> + u64 *cnt)
> +{
> + u32 data;
> + u16 ctrl_addr;
> + u8 check;
> + int loop;
> +
> + ctrl_addr = addr + SWITCH_COUNTER_NUM * port;
> + ctrl_addr |= IND_ACC_TABLE(TABLE_MIB | TABLE_READ);
> +
> + mutex_lock(&dev->alu_mutex);
> + ksz_write16(dev, REG_IND_CTRL_0, ctrl_addr);
> +
> + /* It is almost guaranteed to always read the valid bit because of
> + * slow SPI speed.
> + */
> + for (loop = 2; loop > 0; loop--) {
> + ksz_read8(dev, REG_IND_MIB_CHECK, &check);
> +
> + if (check & MIB_COUNTER_VALID) {
> + ksz_read32(dev, REG_IND_DATA_LO, &data);
> + if (check & MIB_COUNTER_OVERFLOW)
> + *cnt += MIB_COUNTER_VALUE + 1;
> + *cnt += data & MIB_COUNTER_VALUE;
> + break;
> + }
> + }
Hmm. Maybe, but should not this at least warn if if it can not get
valid counter?
> + /* It is almost guaranteed to always read the valid bit because of
> + * slow SPI speed.
> + */
> + for (loop = 2; loop > 0; loop--) {
> + ksz_read8(dev, REG_IND_MIB_CHECK, &check);
> +
> + if (check & MIB_COUNTER_VALID) {
> + ksz_read32(dev, REG_IND_DATA_LO, &data);
> + if (addr < 2) {
> + u64 total;
> +
> + total = check & MIB_TOTAL_BYTES_H;
> + total <<= 32;
> + *cnt += total;
> + *cnt += data;
> + if (check & MIB_COUNTER_OVERFLOW) {
> + total = MIB_TOTAL_BYTES_H + 1;
> + total <<= 32;
> + *cnt += total;
> + }
> + } else {
> + if (check & MIB_COUNTER_OVERFLOW)
> + *cnt += MIB_PACKET_DROPPED + 1;
> + *cnt += data & MIB_PACKET_DROPPED;
> + }
> + break;
> + }
> + }
Same here. Plus, is overflow handling correct? There may be more than
MIB_PACKET_DROPPED + 1 packets dropped between the checks.
> +static void ksz8795_r_table(struct ksz_device *dev, int table, u16 addr,
> + u64 *data)
> +{
> + u16 ctrl_addr;
> +
> + ctrl_addr = IND_ACC_TABLE(table | TABLE_READ) | addr;
> +
> + mutex_lock(&dev->alu_mutex);
> + ksz_write16(dev, REG_IND_CTRL_0, ctrl_addr);
> + ksz_get(dev, REG_IND_DATA_HI, data, sizeof(u64));
> + mutex_unlock(&dev->alu_mutex);
> + *data = be64_to_cpu(*data);
> +}
It would be a tiny bit nicer to have be64 temporary variable and use
it; having *data change endianness at runtime is "interesting".
> +static int ksz8795_valid_dyn_entry(struct ksz_device *dev, u8 *data)
> +{
> + int timeout = 100;
> +
> + do {
> + ksz_read8(dev, REG_IND_DATA_CHECK, data);
> + timeout--;
> + } while ((*data & DYNAMIC_MAC_TABLE_NOT_READY) && timeout);
> +
> + /* Entry is not ready for accessing. */
> + if (*data & DYNAMIC_MAC_TABLE_NOT_READY) {
> + return -EAGAIN;
> + /* Entry is ready for accessing. */
> + } else {
> + ksz_read8(dev, REG_IND_DATA_8, data);
> +
> + /* There is no valid entry in the table. */
> + if (*data & DYNAMIC_MAC_TABLE_MAC_EMPTY)
> + return -ENXIO;
> + }
You can drop else and one indentation level.
> + /* At least one valid entry in the table. */
> + } else {
> + u64 buf;
> + int cnt;
> +
> + ksz_get(dev, REG_IND_DATA_HI, &buf, sizeof(buf));
> + buf = be64_to_cpu(buf);
Would it make sense to convert endianness inside ksz_get?
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: [jkirsher/next-queue PATCH v4 0/6] tc-flower based cloud filters in i40e
From: Jiri Pirko @ 2017-10-11 20:58 UTC (permalink / raw)
To: David Miller
Cc: alexander.duyck, amritha.nambiar, intel-wired-lan,
jeffrey.t.kirsher, alexander.h.duyck, netdev, jhs, xiyou.wangcong
In-Reply-To: <20171011.134652.1653141099248918341.davem@davemloft.net>
Wed, Oct 11, 2017 at 10:46:52PM CEST, davem@davemloft.net wrote:
>From: Jiri Pirko <jiri@resnulli.us>
>Date: Wed, 11 Oct 2017 22:38:32 +0200
>
>> Wed, Oct 11, 2017 at 07:46:27PM CEST, alexander.duyck@gmail.com wrote:
>>>On Wed, Oct 11, 2017 at 5:56 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>>>> Wed, Oct 11, 2017 at 02:24:12AM CEST, amritha.nambiar@intel.com wrote:
>>>>>This patch series enables configuring cloud filters in i40e
>>>>>using the tc-flower classifier. The classification function
>>>>>of the filter is to match a packet to a class. cls_flower is
>>>>>extended to offload classid to hardware. The offloaded classid
>>>>>is used direct matched packets to a traffic class on the device.
>>>>>The approach here is similar to the tc 'prio' qdisc which uses
>>>>>the classid for band selection. The ingress qdisc is called ffff:0,
>>>>>so traffic classes are ffff:1 to ffff:8 (i40e has max of 8 TCs).
>>>>
>>>>
>>>> NACK. This clearly looks like abuse of classid to something
>>>> else. Classid is here to identify qdisc instance. However, you use it
>>>> for hw tclass identification. This is mixing of apples and oranges.
>>>>
>>>> Why?
>>>>
>>>> Please don't try to abuse things! This is not nice.
>>>
>>>This isn't an abuse. This is reproducing in hardware what is already
>>>the behavior for software. Isn't that how offloads are supposed to
>>>work?
>>
>> What is meaning of classid in HW? Classid is SW only identification of
>> qdisc instances. No relation to HW instances = abuse.
>
>Jiri I really don't see what the problem is.
>
>As long as the driver does the right thing when changes are made to the
>qdisc, it doesn't really matter what "key" they use to refer to it.
>
>It could have just as easily used the qdisc pointer and then internally
>use some IDR allocated ID to refer to it in the driver and hardware.
>
>But that's such a waste, we have a unique handle already so why can't
>the driver just use that?
Well if I see classid, I expect it should refer to qdisc instance. So
far, this has been always a case. But for some drivers, this would mean
something totally different and unrelated. So what should I think?
What's next? Classid could be abused to identify something else. I don't
understand why.
classid in kernel and tclass in hw are 2 completely unrelated things.
Why they should share the same userspace api? What am I missing that
indicates this is not an abuse?
There should be clean and well-defined userspace api:
1) classid to identify qdisc instances
2) something else to identify HW tclasses
^ permalink raw reply
* Re: [PATCH v2 net-next 0/2] lan9303: Add basic offloading of unicast traffic
From: David Miller @ 2017-10-11 20:53 UTC (permalink / raw)
To: privat; +Cc: andrew, vivien.didelot, f.fainelli, netdev, linux-kernel
In-Reply-To: <20171010124953.386-1-privat@egil-hjelmeland.no>
From: Egil Hjelmeland <privat@egil-hjelmeland.no>
Date: Tue, 10 Oct 2017 14:49:51 +0200
> This series add basic offloading of unicast traffic to the lan9303
> DSA driver.
>
> Review welcome!
>
> Changes v1 -> v2:
> - Patch 1: Codestyle linting.
> - Patch 2: Remember SWE_PORT_STATE while not bridged.
> Added constant LAN9303_SWE_PORT_MIRROR_DISABLED.
Series applied, thanks.
^ permalink raw reply
* Re: Ethtool question
From: David Miller @ 2017-10-11 20:49 UTC (permalink / raw)
To: linville; +Cc: greearb, netdev
In-Reply-To: <20171011204406.GC30940@tuxdriver.com>
From: "John W. Linville" <linville@tuxdriver.com>
Date: Wed, 11 Oct 2017 16:44:07 -0400
> On Wed, Oct 11, 2017 at 09:51:56AM -0700, Ben Greear wrote:
>> I noticed today that setting some ethtool settings to the same value
>> returns an error code. I would think this should silently return
>> success instead? Makes it easier to call it from scripts this way:
>>
>> [root@lf0313-6477 lanforge]# ethtool -L eth3 combined 1
>> combined unmodified, ignoring
>> no channel parameters changed, aborting
>> current values: tx 0 rx 0 other 1 combined 1
>> [root@lf0313-6477 lanforge]# echo $?
>> 1
>
> I just had this discussion a couple of months ago with someone. My
> initial feeling was like you, a no-op is not a failure. But someone
> convinced me otherwise...I will now endeavour to remember who that
> was and how they convinced me...
>
> Anyone else have input here?
I guess this usually happens when drivers don't support changing the
settings at all. So they just make their ethtool operation for the
'set' always return an error.
We could have a generic ethtool helper that does "get" and then if the
"set" request is identical just return zero.
But from another perspective, the error returned from the "set" in this
situation also indicates to the user that the driver does not support
the "set" operation which has value and meaning in and of itself. And
we'd lose that with the given suggestion.
^ permalink raw reply
* Re: [jkirsher/next-queue PATCH v4 0/6] tc-flower based cloud filters in i40e
From: David Miller @ 2017-10-11 20:46 UTC (permalink / raw)
To: jiri
Cc: alexander.duyck, amritha.nambiar, intel-wired-lan,
jeffrey.t.kirsher, alexander.h.duyck, netdev, jhs, xiyou.wangcong
In-Reply-To: <20171011203832.GA9297@nanopsycho>
From: Jiri Pirko <jiri@resnulli.us>
Date: Wed, 11 Oct 2017 22:38:32 +0200
> Wed, Oct 11, 2017 at 07:46:27PM CEST, alexander.duyck@gmail.com wrote:
>>On Wed, Oct 11, 2017 at 5:56 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>>> Wed, Oct 11, 2017 at 02:24:12AM CEST, amritha.nambiar@intel.com wrote:
>>>>This patch series enables configuring cloud filters in i40e
>>>>using the tc-flower classifier. The classification function
>>>>of the filter is to match a packet to a class. cls_flower is
>>>>extended to offload classid to hardware. The offloaded classid
>>>>is used direct matched packets to a traffic class on the device.
>>>>The approach here is similar to the tc 'prio' qdisc which uses
>>>>the classid for band selection. The ingress qdisc is called ffff:0,
>>>>so traffic classes are ffff:1 to ffff:8 (i40e has max of 8 TCs).
>>>
>>>
>>> NACK. This clearly looks like abuse of classid to something
>>> else. Classid is here to identify qdisc instance. However, you use it
>>> for hw tclass identification. This is mixing of apples and oranges.
>>>
>>> Why?
>>>
>>> Please don't try to abuse things! This is not nice.
>>
>>This isn't an abuse. This is reproducing in hardware what is already
>>the behavior for software. Isn't that how offloads are supposed to
>>work?
>
> What is meaning of classid in HW? Classid is SW only identification of
> qdisc instances. No relation to HW instances = abuse.
Jiri I really don't see what the problem is.
As long as the driver does the right thing when changes are made to the
qdisc, it doesn't really matter what "key" they use to refer to it.
It could have just as easily used the qdisc pointer and then internally
use some IDR allocated ID to refer to it in the driver and hardware.
But that's such a waste, we have a unique handle already so why can't
the driver just use that?
^ permalink raw reply
* Re: [PATCH RFC] Add other KSZ switch support so that patch check does not complain
From: Pavel Machek @ 2017-10-11 20:46 UTC (permalink / raw)
To: Tristram.Ha
Cc: Andrew Lunn, Florian Fainelli, Ruediger Schmitt, muvarov,
nathan.leigh.conrad, vivien.didelot, UNGLinuxDriver, netdev,
linux-kernel
In-Reply-To: <1507322009-15142-1-git-send-email-Tristram.Ha@microchip.com>
[-- Attachment #1: Type: text/plain, Size: 433 bytes --]
On Fri 2017-10-06 13:33:29, Tristram.Ha@microchip.com wrote:
> From: Tristram Ha <Tristram.Ha@microchip.com>
>
> Add other KSZ switch support so that patch check does not complain.
>
> Signed-off-by: Tristram Ha <Tristram.Ha@microchip.com>
Reviewed-by: Pavel Machek <pavel@ucw.cz>
--
(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: [PATCH v1 RFC 7/7] Modify tag_ksz.c so that tail tag code can be used by other KSZ switch drivers
From: Pavel Machek @ 2017-10-11 20:45 UTC (permalink / raw)
To: Tristram.Ha
Cc: Andrew Lunn, Florian Fainelli, Ruediger Schmitt, muvarov,
nathan.leigh.conrad, vivien.didelot, UNGLinuxDriver, netdev,
linux-kernel
In-Reply-To: <1507321985-15097-8-git-send-email-Tristram.Ha@microchip.com>
[-- Attachment #1: Type: text/plain, Size: 1690 bytes --]
Hi!
> +#define KSZ_INGRESS_TAG_LEN 1
This define is now (or should be) unused, so you can delete it, no?
> _#define KSZ_EGRESS_TAG_LEN 1
And I'd delete this define, too. Having constant for something that's
variable is quite confusing :-).
Plus you are really doing too much inside single patch.
> + * For Egress (KSZ9477 -> Host), 1 byte is added before FCS.
> + * ---------------------------------------------------------------------------
> + * DA(6bytes)|SA(6bytes)|....|Data(nbytes)|tag0(1byte)|FCS(4bytes)
> + * ---------------------------------------------------------------------------
> + * tag0 : zero-based value represents port
> + * (eg, 0x00=port1, 0x02=port3, 0x06=port7)
> + */
> +
> +#define KSZ9477_INGRESS_TAG_LEN 2
> +#define KSZ9477_PTP_TAG_LEN 4
> +#define KSZ9477_PTP_TAG_INDICATION 0x80
> +
> +#define KSZ9477_TAIL_TAG_OVERRIDE BIT(9)
> +#define KSZ9477_TAIL_TAG_LOOKUP BIT(10)
> +
> +static int ksz9477_get_tag(u8 *tag, int *port)
> +{
> + int len = KSZ_EGRESS_TAG_LEN;
> +
> + /* Extra 4-bytes PTP timestamp */
> + if (tag[0] & KSZ9477_PTP_TAG_INDICATION)
> + len += KSZ9477_PTP_TAG_LEN;
> + *port = tag[0] & 7;
> + return len;
> +}
> +
> +static void ksz9477_set_tag(void *ptr, u8 *addr, int p)
> +{
> + u16 *tag = (u16 *)ptr;
> +
> + *tag = 1 << p;
> + if (!memcmp(addr, special_mult_addr, ETH_ALEN))
> + *tag |= KSZ9477_TAIL_TAG_OVERRIDE;
> + *tag = cpu_to_be16(*tag);
> +}
These are new features that were not there before, right?
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: Ethtool question
From: John W. Linville @ 2017-10-11 20:44 UTC (permalink / raw)
To: Ben Greear; +Cc: netdev
In-Reply-To: <492e57b2-ac0a-ff01-2698-e048e97d8e37@candelatech.com>
On Wed, Oct 11, 2017 at 09:51:56AM -0700, Ben Greear wrote:
> I noticed today that setting some ethtool settings to the same value
> returns an error code. I would think this should silently return
> success instead? Makes it easier to call it from scripts this way:
>
> [root@lf0313-6477 lanforge]# ethtool -L eth3 combined 1
> combined unmodified, ignoring
> no channel parameters changed, aborting
> current values: tx 0 rx 0 other 1 combined 1
> [root@lf0313-6477 lanforge]# echo $?
> 1
I just had this discussion a couple of months ago with someone. My
initial feeling was like you, a no-op is not a failure. But someone
convinced me otherwise...I will now endeavour to remember who that
was and how they convinced me...
Anyone else have input here?
John
--
John W. Linville Someday the world will need a hero, and you
linville@tuxdriver.com might be all we have. Be ready.
^ permalink raw reply
* Re: [patch net-next 1/4] net: sched: make tc_action_ops->get_dev return dev and avoid passing net
From: Jiri Pirko @ 2017-10-11 20:43 UTC (permalink / raw)
To: Cong Wang
Cc: Linux Kernel Network Developers, David Miller, Jamal Hadi Salim,
Saeed Mahameed, matanb, leonro, mlxsw
In-Reply-To: <CAM_iQpU5MP546FjwdD_PtOUFiAKb_TiRBEY3b+Kt9mVcCY7MTQ@mail.gmail.com>
Wed, Oct 11, 2017 at 06:34:51PM CEST, xiyou.wangcong@gmail.com wrote:
>On Tue, Oct 10, 2017 at 2:19 PM, Jiri Pirko <jiri@resnulli.us> wrote:
>> Tue, Oct 10, 2017 at 07:44:53PM CEST, xiyou.wangcong@gmail.com wrote:
>>>On Tue, Oct 10, 2017 at 12:30 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>>>> -static int tcf_mirred_device(const struct tc_action *a, struct net *net,
>>>> - struct net_device **mirred_dev)
>>>> +static struct net_device *tcf_mirred_get_dev(const struct tc_action *a)
>>>> {
>>>> - int ifindex = tcf_mirred_ifindex(a);
>>>> + struct tcf_mirred *m = to_mirred(a);
>>>>
>>>> - *mirred_dev = __dev_get_by_index(net, ifindex);
>>>> - if (!*mirred_dev)
>>>> - return -EINVAL;
>>>> - return 0;
>>>> + return __dev_get_by_index(m->net, m->tcfm_ifindex);
>>>
>>>Hmm, why not just return m->tcfm_dev?
>>
>> I just follow the existing code. The change you suggest should be a
>> separate follow-up patch.
>
>Why?
I try to do small contained changes per patch. The resulting code is
doing the same thing as the original, therefore reducing possible bug
appearance.
>
>Your goal is "make tc_action_ops->get_dev return dev and avoid passing net",
>using m->tcfm_dev is simpler and could save you from adding a net pointer
>to struct tcf_mirred too.
^ permalink raw reply
* Re: [PATCH net-next v3 5/5] selinux: bpf: Add addtional check for bpf object file receive
From: Chenbo Feng @ 2017-10-11 20:43 UTC (permalink / raw)
To: Stephen Smalley
Cc: Chenbo Feng, linux-security-module, netdev, SELinux,
Daniel Borkmann, Alexei Starovoitov, Lorenzo Colitti
In-Reply-To: <1507726470.15898.3.camel@tycho.nsa.gov>
On Wed, Oct 11, 2017 at 5:54 AM, Stephen Smalley <sds@tycho.nsa.gov> wrote:
> On Tue, 2017-10-10 at 17:09 -0700, Chenbo Feng wrote:
>> From: Chenbo Feng <fengc@google.com>
>>
>> Introduce a bpf object related check when sending and receiving files
>> through unix domain socket as well as binder. It checks if the
>> receiving
>> process have privilege to read/write the bpf map or use the bpf
>> program.
>> This check is necessary because the bpf maps and programs are using a
>> anonymous inode as their shared inode so the normal way of checking
>> the
>> files and sockets when passing between processes cannot work properly
>> on
>> eBPF object. This check only works when the BPF_SYSCALL is
>> configured.
>> The information stored inside the file security struct is the same as
>> the information in bpf object security struct.
>>
>> Signed-off-by: Chenbo Feng <fengc@google.com>
>> ---
>> include/linux/lsm_hooks.h | 17 ++++++++++
>> include/linux/security.h | 9 ++++++
>> kernel/bpf/syscall.c | 27 ++++++++++++++--
>> security/security.c | 8 +++++
>> security/selinux/hooks.c | 67
>> +++++++++++++++++++++++++++++++++++++++
>> security/selinux/include/objsec.h | 9 ++++++
>> 6 files changed, 135 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
>> index 7161d8e7ee79..517dea60b87b 100644
>> --- a/include/linux/lsm_hooks.h
>> +++ b/include/linux/lsm_hooks.h
>> @@ -1385,6 +1385,19 @@
>> * @bpf_prog_free_security:
>> * Clean up the security information stored inside bpf prog.
>> *
>> + * @bpf_map_file:
>> + * When creating a bpf map fd, set up the file security
>> information with
>> + * the bpf security information stored in the map struct. So
>> when the map
>> + * fd is passed between processes, the security module can
>> directly read
>> + * the security information from file security struct rather
>> than the bpf
>> + * security struct.
>> + *
>> + * @bpf_prog_file:
>> + * When creating a bpf prog fd, set up the file security
>> information with
>> + * the bpf security information stored in the prog struct. So
>> when the prog
>> + * fd is passed between processes, the security module can
>> directly read
>> + * the security information from file security struct rather
>> than the bpf
>> + * security struct.
>> */
>> union security_list_options {
>> int (*binder_set_context_mgr)(struct task_struct *mgr);
>> @@ -1726,6 +1739,8 @@ union security_list_options {
>> void (*bpf_map_free_security)(struct bpf_map *map);
>> int (*bpf_prog_alloc_security)(struct bpf_prog_aux *aux);
>> void (*bpf_prog_free_security)(struct bpf_prog_aux *aux);
>> + void (*bpf_map_file)(struct bpf_map *map, struct file
>> *file);
>> + void (*bpf_prog_file)(struct bpf_prog_aux *aux, struct file
>> *file);
>> #endif /* CONFIG_BPF_SYSCALL */
>> };
>>
>> @@ -1954,6 +1969,8 @@ struct security_hook_heads {
>> struct list_head bpf_map_free_security;
>> struct list_head bpf_prog_alloc_security;
>> struct list_head bpf_prog_free_security;
>> + struct list_head bpf_map_file;
>> + struct list_head bpf_prog_file;
>> #endif /* CONFIG_BPF_SYSCALL */
>> } __randomize_layout;
>>
>> diff --git a/include/linux/security.h b/include/linux/security.h
>> index 18800b0911e5..57573b794e2d 100644
>> --- a/include/linux/security.h
>> +++ b/include/linux/security.h
>> @@ -1740,6 +1740,8 @@ extern int security_bpf_map_alloc(struct
>> bpf_map *map);
>> extern void security_bpf_map_free(struct bpf_map *map);
>> extern int security_bpf_prog_alloc(struct bpf_prog_aux *aux);
>> extern void security_bpf_prog_free(struct bpf_prog_aux *aux);
>> +extern void security_bpf_map_file(struct bpf_map *map, struct file
>> *file);
>> +extern void security_bpf_prog_file(struct bpf_prog_aux *aux, struct
>> file *file);
>> #else
>> static inline int security_bpf(int cmd, union bpf_attr *attr,
>> unsigned int size)
>> @@ -1772,6 +1774,13 @@ static inline int
>> security_bpf_prog_alloc(struct bpf_prog_aux *aux)
>>
>> static inline void security_bpf_prog_free(struct bpf_prog_aux *aux)
>> { }
>> +
>> +static inline void security_bpf_map_file(struct bpf_map *map, struct
>> file *file)
>> +{ }
>> +
>> +static inline void security_bpf_prog_file(struct bpf_prog_aux *aux,
>> + struct file *file)
>> +{ }
>> #endif /* CONFIG_SECURITY */
>> #endif /* CONFIG_BPF_SYSCALL */
>>
>> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
>> index 1cf31ddd7616..aee69e564c50 100644
>> --- a/kernel/bpf/syscall.c
>> +++ b/kernel/bpf/syscall.c
>> @@ -324,11 +324,22 @@ static const struct file_operations
>> bpf_map_fops = {
>>
>> int bpf_map_new_fd(struct bpf_map *map, int flags)
>> {
>> + int fd;
>> + struct fd f;
>> if (security_bpf_map(map, OPEN_FMODE(flags)))
>> return -EPERM;
>>
>> - return anon_inode_getfd("bpf-map", &bpf_map_fops, map,
>> + fd = anon_inode_getfd("bpf-map", &bpf_map_fops, map,
>> flags | O_CLOEXEC);
>> + if (fd < 0)
>> + return fd;
>> +
>> + f = fdget(fd);
>> + if (!f.file)
>> + return -EBADF;
>
> This seems convoluted and unnecessarily inefficient, since
> anon_inode_getfd() has the struct file and could have directly returned
> it instead of having to go through fdget() on a fd we just installed.
> Also, couldn't the fd->file mapping have changed underneath us between
> fd_install() and fdget()?
> I would think it would be safer and more efficient to create an
> anon_inode_getfdandfile() or similar interface and use that, so that we
> can just pass the file it set up to the hook. Obviously that would
> need to be reviewed by the vfs folks.
>
Do you mean create a anonymous inode interface specifically for eBPF
object? Is it okay that we add the hooks inside anon_inode_getfd and
pass the file to the hook before fd install.
>> + security_bpf_map_file(map, f.file);
>> + fdput(f);
>> + return fd;
>> }
>>
>> int bpf_get_file_flag(int flags)
>> @@ -975,11 +986,23 @@ static const struct file_operations
>> bpf_prog_fops = {
>>
>> int bpf_prog_new_fd(struct bpf_prog *prog)
>> {
>> + int fd;
>> + struct fd f;
>> +
>> if (security_bpf_prog(prog))
>> return -EPERM;
>>
>> - return anon_inode_getfd("bpf-prog", &bpf_prog_fops, prog,
>> + fd = anon_inode_getfd("bpf-prog", &bpf_prog_fops, prog,
>> O_RDWR | O_CLOEXEC);
>> + if (fd < 0)
>> + return fd;
>> +
>> + f = fdget(fd);
>> + if (!f.file)
>> + return -EBADF;
>> + security_bpf_prog_file(prog->aux, f.file);
>> + fdput(f);
>> + return fd;
>> }
>>
>> static struct bpf_prog *____bpf_prog_get(struct fd f)
>> diff --git a/security/security.c b/security/security.c
>> index 1cd8526cb0b7..dacf649b8cfa 100644
>> --- a/security/security.c
>> +++ b/security/security.c
>> @@ -1734,4 +1734,12 @@ void security_bpf_prog_free(struct
>> bpf_prog_aux *aux)
>> {
>> call_void_hook(bpf_prog_free_security, aux);
>> }
>> +void security_bpf_map_file(struct bpf_map *map, struct file *file)
>> +{
>> + call_void_hook(bpf_map_file, map, file);
>> +}
>> +void security_bpf_prog_file(struct bpf_prog_aux *aux, struct file
>> *file)
>> +{
>> + call_void_hook(bpf_prog_file, aux, file);
>> +}
>> #endif /* CONFIG_BPF_SYSCALL */
>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>> index 94e473b9c884..0a6ef20513b0 100644
>> --- a/security/selinux/hooks.c
>> +++ b/security/selinux/hooks.c
>> @@ -1815,6 +1815,10 @@ static inline int file_path_has_perm(const
>> struct cred *cred,
>> return inode_has_perm(cred, file_inode(file), av, &ad);
>> }
>>
>> +#ifdef CONFIG_BPF_SYSCALL
>> +static int bpf_file_check(struct file *file, u32 sid);
>> +#endif
>> +
>> /* Check whether a task can use an open file descriptor to
>> access an inode in a given way. Check access to the
>> descriptor itself, and then use dentry_has_perm to
>> @@ -1845,6 +1849,14 @@ static int file_has_perm(const struct cred
>> *cred,
>> goto out;
>> }
>>
>> +#ifdef CONFIG_BPF_SYSCALL
>> + if (fsec->bpf_type) {
>> + rc = bpf_file_check(file, cred_sid(cred));
>> + if (rc)
>> + goto out;
>> + }
>> +#endif
>> +
>> /* av is zero if only checking access to the descriptor. */
>> rc = 0;
>> if (av)
>> @@ -2165,6 +2177,14 @@ static int selinux_binder_transfer_file(struct
>> task_struct *from,
>> return rc;
>> }
>>
>> +#ifdef CONFIG_BPF_SYSCALL
>> + if (fsec->bpf_type) {
>> + rc = bpf_file_check(file, sid);
>> + if (rc)
>> + return rc;
>> + }
>> +#endif
>> +
>> if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))
>> return 0;
>>
>> @@ -6288,6 +6308,33 @@ static u32 bpf_map_fmode_to_av(fmode_t fmode)
>> return av;
>> }
>>
>> +/* This function will check the file pass through unix socket or
>> binder to see
>> + * if it is a bpf related object. And apply correspinding checks on
>> the bpf
>> + * object based on the type. The bpf maps and programs, not like
>> other files and
>> + * socket, are using a shared anonymous inode inside the kernel as
>> their inode.
>> + * So checking that inode cannot identify if the process have
>> privilege to
>> + * access the bpf object and that's why we have to add this
>> additional check in
>> + * selinux_file_receive and selinux_binder_transfer_files.
>> + */
>> +static int bpf_file_check(struct file *file, u32 sid)
>> +{
>> + struct file_security_struct *fsec = file->f_security;
>> + int ret;
>> +
>> + if (fsec->bpf_type == BPF_MAP) {
>> + ret = avc_has_perm(sid, fsec->bpf_sid, SECCLASS_BPF,
>> + bpf_map_fmode_to_av(file-
>> >f_mode), NULL);
>> + if (ret)
>> + return ret;
>> + } else if (fsec->bpf_type == BPF_PROG) {
>> + ret = avc_has_perm(sid, fsec->bpf_sid, SECCLASS_BPF,
>> + BPF__PROG_USE, NULL);
>> + if (ret)
>> + return ret;
>> + }
>> + return 0;
>> +}
>> +
>> static int selinux_bpf_map(struct bpf_map *map, fmode_t fmode)
>> {
>> u32 sid = current_sid();
>> @@ -6351,6 +6398,24 @@ static void selinux_bpf_prog_free(struct
>> bpf_prog_aux *aux)
>> aux->security = NULL;
>> kfree(bpfsec);
>> }
>> +
>> +static void selinux_bpf_map_file(struct bpf_map *map, struct file
>> *file)
>> +{
>> + struct bpf_security_struct *bpfsec = map->security;
>> + struct file_security_struct *fsec = file->f_security;
>> +
>> + fsec->bpf_type = BPF_MAP;
>> + fsec->bpf_sid = bpfsec->sid;
>> +}
>> +
>> +static void selinux_bpf_prog_file(struct bpf_prog_aux *aux, struct
>> file *file)
>> +{
>> + struct bpf_security_struct *bpfsec = aux->security;
>> + struct file_security_struct *fsec = file->f_security;
>> +
>> + fsec->bpf_type = BPF_PROG;
>> + fsec->bpf_sid = bpfsec->sid;
>> +}
>> #endif
>>
>> static struct security_hook_list selinux_hooks[] __lsm_ro_after_init
>> = {
>> @@ -6581,6 +6646,8 @@ static struct security_hook_list
>> selinux_hooks[] __lsm_ro_after_init = {
>> LSM_HOOK_INIT(bpf_prog_alloc_security,
>> selinux_bpf_prog_alloc),
>> LSM_HOOK_INIT(bpf_map_free_security, selinux_bpf_map_free),
>> LSM_HOOK_INIT(bpf_prog_free_security,
>> selinux_bpf_prog_free),
>> + LSM_HOOK_INIT(bpf_map_file, selinux_bpf_map_file),
>> + LSM_HOOK_INIT(bpf_prog_file, selinux_bpf_prog_file),
>> #endif
>> };
>>
>> diff --git a/security/selinux/include/objsec.h
>> b/security/selinux/include/objsec.h
>> index 3d54468ce334..0162648761f9 100644
>> --- a/security/selinux/include/objsec.h
>> +++ b/security/selinux/include/objsec.h
>> @@ -67,11 +67,20 @@ struct inode_security_struct {
>> spinlock_t lock;
>> };
>>
>> +enum bpf_obj_type {
>> + BPF_MAP = 1,
>> + BPF_PROG,
>> +};
>> +
>> struct file_security_struct {
>> u32 sid; /* SID of open file description */
>> u32 fown_sid; /* SID of file owner (for
>> SIGIO) */
>> u32 isid; /* SID of inode at the time of file
>> open */
>> u32 pseqno; /* Policy seqno at the time of
>> file open */
>> +#ifdef CONFIG_BPF_SYSCALL
>> + unsigned char bpf_type;
>> + u32 bpf_sid;
>> +#endif
>> };
>
> Can you check how this impacts the size of the file_security_cache
> objects, and thus the memory overhead imposed on all open files?
>
> If it is significant, do we need to cache the bpf_sid here or could we
> just store the bpf_type and then dereference the bpfsec if it is a map
> or prog?
>
>From proc/slabinfo I find the number of object and the object size
grows a lot after adding this two field. I will try to dereference the
bpfsec instead to see if it helps.
>>
>> struct superblock_security_struct {
^ permalink raw reply
* Re: [PATCH net-next] tcp: fix tcp_unlink_write_queue()
From: David Miller @ 2017-10-11 20:41 UTC (permalink / raw)
To: eric.dumazet; +Cc: ynorov, netdev, catalin.marinas, weiwan, ncardwell
In-Reply-To: <1507753649.31614.36.camel@edumazet-glaptop3.roam.corp.google.com>
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Wed, 11 Oct 2017 13:27:29 -0700
> From: Eric Dumazet <edumazet@google.com>
>
> Yury reported crash with this signature :
>
> [ 554.034021] [<ffff80003ccd5a58>] 0xffff80003ccd5a58
> [ 554.034156] [<ffff00000888fd34>] skb_release_all+0x14/0x30
> [ 554.034288] [<ffff00000888fd64>] __kfree_skb+0x14/0x28
> [ 554.034409] [<ffff0000088ece6c>] tcp_sendmsg_locked+0x4dc/0xcc8
> [ 554.034541] [<ffff0000088ed68c>] tcp_sendmsg+0x34/0x58
> [ 554.034659] [<ffff000008919fd4>] inet_sendmsg+0x2c/0xf8
> [ 554.034783] [<ffff0000088842e8>] sock_sendmsg+0x18/0x30
> [ 554.034928] [<ffff0000088861fc>] SyS_sendto+0x84/0xf8
>
> Problem is that skb->destructor contains garbage, and this is
> because I accidentally removed tcp_skb_tsorted_anchor_cleanup()
> from tcp_unlink_write_queue()
>
> This would trigger with a write(fd, <invalid_memory>, len) attempt,
> and we will add to packetdrill this capability to avoid future
> regressions.
>
> Fixes: 75c119afe14f ("tcp: implement rb-tree based retransmit queue")
> Reported-by: Yury Norov <ynorov@caviumnetworks.com>
> Tested-by: Yury Norov <ynorov@caviumnetworks.com>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
Applied, thanks Eric.
^ permalink raw reply
* Re: [PATCH net-next 2/2] net sched act_vlan: VLAN action rewrite to use RCU lock/unlock and update
From: Jiri Pirko @ 2017-10-11 20:41 UTC (permalink / raw)
To: Cong Wang
Cc: Manish Kurup, Jamal Hadi Salim, David Miller,
Linux Kernel Network Developers, LKML, Alexander Aring,
Roman Mashak, manish.kurup
In-Reply-To: <CAM_iQpUYGUzvvM3Weaw5ertgVpwn0p9j6B5xd5Ar5ysJwrdVvg@mail.gmail.com>
Wed, Oct 11, 2017 at 06:27:07PM CEST, xiyou.wangcong@gmail.com wrote:
>On Tue, Oct 10, 2017 at 7:33 PM, Manish Kurup <kurup.manish@gmail.com> wrote:
[...]
>> @@ -187,16 +196,33 @@ static int tcf_vlan_init(struct net *net, struct nlattr *nla,
>>
>> v = to_vlan(*a);
>>
>> - spin_lock_bh(&v->tcf_lock);
>> -
>> - v->tcfv_action = action;
>> - v->tcfv_push_vid = push_vid;
>> - v->tcfv_push_prio = push_prio;
>> - v->tcfv_push_proto = push_proto;
>> + ASSERT_RTNL();
>> + p = kzalloc(sizeof(*p), GFP_KERNEL);
>> + if (unlikely(!p)) {
>> + if (ovr)
>> + tcf_idr_release(*a, bind);
>> + return -ENOMEM;
>> + }
>>
>> v->tcf_action = parm->action;
>>
>> - spin_unlock_bh(&v->tcf_lock);
>> + p_old = rtnl_dereference(v->vlan_p);
>> +
>> + if (ovr)
>> + spin_lock_bh(&v->tcf_lock);
>
>Why still take spinlock when you already have RTNL lock?
>What's the point?
Yeah, I believe this is copy&paste bug from act_skbmod
^ permalink raw reply
* Re: [jkirsher/next-queue PATCH v4 0/6] tc-flower based cloud filters in i40e
From: Jiri Pirko @ 2017-10-11 20:38 UTC (permalink / raw)
To: Alexander Duyck
Cc: Amritha Nambiar, intel-wired-lan, Jeff Kirsher,
Duyck, Alexander H, Netdev, Jamal Hadi Salim, Cong Wang
In-Reply-To: <CAKgT0UcP8refj2KLMT258xPNS2h3KhLqKD5sJkNLXRdNJrRCVg@mail.gmail.com>
Wed, Oct 11, 2017 at 07:46:27PM CEST, alexander.duyck@gmail.com wrote:
>On Wed, Oct 11, 2017 at 5:56 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>> Wed, Oct 11, 2017 at 02:24:12AM CEST, amritha.nambiar@intel.com wrote:
>>>This patch series enables configuring cloud filters in i40e
>>>using the tc-flower classifier. The classification function
>>>of the filter is to match a packet to a class. cls_flower is
>>>extended to offload classid to hardware. The offloaded classid
>>>is used direct matched packets to a traffic class on the device.
>>>The approach here is similar to the tc 'prio' qdisc which uses
>>>the classid for band selection. The ingress qdisc is called ffff:0,
>>>so traffic classes are ffff:1 to ffff:8 (i40e has max of 8 TCs).
>>
>>
>> NACK. This clearly looks like abuse of classid to something
>> else. Classid is here to identify qdisc instance. However, you use it
>> for hw tclass identification. This is mixing of apples and oranges.
>>
>> Why?
>>
>> Please don't try to abuse things! This is not nice.
>
>This isn't an abuse. This is reproducing in hardware what is already
>the behavior for software. Isn't that how offloads are supposed to
>work?
What is meaning of classid in HW? Classid is SW only identification of
qdisc instances. No relation to HW instances = abuse.
>
>This is exactly how prio currently handles this. We are essentially
>doing the exact same thing in the hardware where we are choosing a
>queueing group based on the class ID. You could setup a prio qdisc. If
>you are offloading a qdisc behavior into hardware how are you supposed
>to emulate the behavior if you aren't allowing the offload to use the
>same mechanism?
I believe that the correct way to solve is to introduce SETTCLASS
termination action (similar to TRAP, GOTO_CHAIN, etc), which would allow
user to instruct the destination tclass and driver can offload it.
^ permalink raw reply
* Re: next: arm64: LTP sendto01 test causes system crash in ilp32 mode
From: Eric Dumazet @ 2017-10-11 20:28 UTC (permalink / raw)
To: Yury Norov
Cc: linux-kernel, linux-arm-kernel, netdev, Catalin Marinas,
David S. Miller, Florian Westphal
In-Reply-To: <20171011194355.n6mb4oe7qt76d6wh@yury-thinkpad>
On Wed, 2017-10-11 at 22:43 +0300, Yury Norov wrote:
> The fix works for me, thanks.
>
> Tested-by: Yury Norov <ynorov@caviumnetworks.com>
Thanks Yury !
^ permalink raw reply
* [PATCH net-next] tcp: fix tcp_unlink_write_queue()
From: Eric Dumazet @ 2017-10-11 20:27 UTC (permalink / raw)
To: Yury Norov
Cc: netdev, Catalin Marinas, David S. Miller, Wei Wang, Neal Cardwell
In-Reply-To: <20171011194355.n6mb4oe7qt76d6wh@yury-thinkpad>
From: Eric Dumazet <edumazet@google.com>
Yury reported crash with this signature :
[ 554.034021] [<ffff80003ccd5a58>] 0xffff80003ccd5a58
[ 554.034156] [<ffff00000888fd34>] skb_release_all+0x14/0x30
[ 554.034288] [<ffff00000888fd64>] __kfree_skb+0x14/0x28
[ 554.034409] [<ffff0000088ece6c>] tcp_sendmsg_locked+0x4dc/0xcc8
[ 554.034541] [<ffff0000088ed68c>] tcp_sendmsg+0x34/0x58
[ 554.034659] [<ffff000008919fd4>] inet_sendmsg+0x2c/0xf8
[ 554.034783] [<ffff0000088842e8>] sock_sendmsg+0x18/0x30
[ 554.034928] [<ffff0000088861fc>] SyS_sendto+0x84/0xf8
Problem is that skb->destructor contains garbage, and this is
because I accidentally removed tcp_skb_tsorted_anchor_cleanup()
from tcp_unlink_write_queue()
This would trigger with a write(fd, <invalid_memory>, len) attempt,
and we will add to packetdrill this capability to avoid future
regressions.
Fixes: 75c119afe14f ("tcp: implement rb-tree based retransmit queue")
Reported-by: Yury Norov <ynorov@caviumnetworks.com>
Tested-by: Yury Norov <ynorov@caviumnetworks.com>
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
include/net/tcp.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/include/net/tcp.h b/include/net/tcp.h
index 5a95e5886b55e03e4a8bfeac3506c657a4f97dde..15163454174babdcb465904f725b919268dd1bc7 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -1712,6 +1712,7 @@ static inline void tcp_insert_write_queue_before(struct sk_buff *new,
static inline void tcp_unlink_write_queue(struct sk_buff *skb, struct sock *sk)
{
+ tcp_skb_tsorted_anchor_cleanup(skb);
__skb_unlink(skb, &sk->sk_write_queue);
}
^ permalink raw reply related
* (unknown),
From: kindergartenchaos2 @ 2017-10-11 19:55 UTC (permalink / raw)
To: netdev
[-- Attachment #1: 150646856.zip --]
[-- Type: application/zip, Size: 2805 bytes --]
^ permalink raw reply
* [PATCH 3/4] net: qcom/emac: enforce DMA address restrictions
From: Timur Tabi @ 2017-10-11 19:52 UTC (permalink / raw)
To: David S. Miller, netdev; +Cc: timur
In-Reply-To: <1507751546-10265-1-git-send-email-timur@codeaurora.org>
The EMAC has a restriction that the upper 32 bits of the base addresses
for the RFD and RRD rings must be the same. The ensure that restriction,
we allocate twice the space for the RRD and locate it at an appropriate
address.
We also re-arrange the allocations so that invalid addresses are even
less likely.
Signed-off-by: Timur Tabi <timur@codeaurora.org>
---
drivers/net/ethernet/qualcomm/emac/emac-mac.c | 39 ++++++++++++++++-----------
1 file changed, 24 insertions(+), 15 deletions(-)
diff --git a/drivers/net/ethernet/qualcomm/emac/emac-mac.c b/drivers/net/ethernet/qualcomm/emac/emac-mac.c
index 9cbb27263742..0f5ece5d9507 100644
--- a/drivers/net/ethernet/qualcomm/emac/emac-mac.c
+++ b/drivers/net/ethernet/qualcomm/emac/emac-mac.c
@@ -734,6 +734,11 @@ static int emac_rx_descs_alloc(struct emac_adapter *adpt)
rx_q->rrd.size = rx_q->rrd.count * (adpt->rrd_size * 4);
rx_q->rfd.size = rx_q->rfd.count * (adpt->rfd_size * 4);
+ /* Check if the RRD and RFD are aligned properly, and if not, adjust. */
+ if (upper_32_bits(ring_header->dma_addr) !=
+ upper_32_bits(ring_header->dma_addr + ALIGN(rx_q->rrd.size, 8)))
+ ring_header->used = ALIGN(rx_q->rrd.size, 8);
+
rx_q->rrd.dma_addr = ring_header->dma_addr + ring_header->used;
rx_q->rrd.v_addr = ring_header->v_addr + ring_header->used;
ring_header->used += ALIGN(rx_q->rrd.size, 8);
@@ -767,11 +772,18 @@ int emac_mac_rx_tx_rings_alloc_all(struct emac_adapter *adpt)
/* Ring DMA buffer. Each ring may need up to 8 bytes for alignment,
* hence the additional padding bytes are allocated.
+ *
+ * Also double the memory allocated for the RRD so that we can
+ * re-align it if necessary. The EMAC has a restriction that the
+ * upper 32 bits of the base addresses for the RFD and RRD rings
+ * must be the same. It is extremely unlikely that this is not the
+ * case, since the rings are only a few KB in size. However, we
+ * need to check for this anyway, and if the two rings are not
+ * compliant, then we re-align.
*/
- ring_header->size = num_tx_descs * (adpt->tpd_size * 4) +
- num_rx_descs * (adpt->rfd_size * 4) +
- num_rx_descs * (adpt->rrd_size * 4) +
- 8 + 2 * 8; /* 8 byte per one Tx and two Rx rings */
+ ring_header->size = ALIGN(num_tx_descs * (adpt->tpd_size * 4), 8) +
+ ALIGN(num_rx_descs * (adpt->rfd_size * 4), 8) +
+ ALIGN(num_rx_descs * (adpt->rrd_size * 4), 8) * 2;
ring_header->used = 0;
ring_header->v_addr = dma_zalloc_coherent(dev, ring_header->size,
@@ -780,26 +792,23 @@ int emac_mac_rx_tx_rings_alloc_all(struct emac_adapter *adpt)
if (!ring_header->v_addr)
return -ENOMEM;
- ring_header->used = ALIGN(ring_header->dma_addr, 8) -
- ring_header->dma_addr;
-
- ret = emac_tx_q_desc_alloc(adpt, &adpt->tx_q);
- if (ret) {
- netdev_err(adpt->netdev, "error: Tx Queue alloc failed\n");
- goto err_alloc_tx;
- }
-
ret = emac_rx_descs_alloc(adpt);
if (ret) {
netdev_err(adpt->netdev, "error: Rx Queue alloc failed\n");
goto err_alloc_rx;
}
+ ret = emac_tx_q_desc_alloc(adpt, &adpt->tx_q);
+ if (ret) {
+ netdev_err(adpt->netdev, "transmit queue allocation failed\n");
+ goto err_alloc_tx;
+ }
+
return 0;
-err_alloc_rx:
- emac_tx_q_bufs_free(adpt);
err_alloc_tx:
+ emac_rx_q_bufs_free(adpt);
+err_alloc_rx:
dma_free_coherent(dev, ring_header->size,
ring_header->v_addr, ring_header->dma_addr);
--
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc. Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.
^ permalink raw reply related
* [PATCH 4/4] net: qcom/emac: clean up some TX/RX error messages
From: Timur Tabi @ 2017-10-11 19:52 UTC (permalink / raw)
To: David S. Miller, netdev; +Cc: timur
In-Reply-To: <1507751546-10265-1-git-send-email-timur@codeaurora.org>
Some of the error messages that are printed by the interrupt handlers
are poorly written. For example, many don't include a device prefix,
so there's no indication that they are EMAC errors.
Also use rate limiting for all messages that could be printed from
interrupt context.
Signed-off-by: Timur Tabi <timur@codeaurora.org>
---
drivers/net/ethernet/qualcomm/emac/emac-sgmii.c | 15 ++++++---------
drivers/net/ethernet/qualcomm/emac/emac.c | 8 ++++----
2 files changed, 10 insertions(+), 13 deletions(-)
diff --git a/drivers/net/ethernet/qualcomm/emac/emac-sgmii.c b/drivers/net/ethernet/qualcomm/emac/emac-sgmii.c
index 29ba37a08372..e8ab512ee7e3 100644
--- a/drivers/net/ethernet/qualcomm/emac/emac-sgmii.c
+++ b/drivers/net/ethernet/qualcomm/emac/emac-sgmii.c
@@ -68,10 +68,10 @@ static void emac_sgmii_link_init(struct emac_adapter *adpt)
writel(val, phy->base + EMAC_SGMII_PHY_AUTONEG_CFG2);
}
-static int emac_sgmii_irq_clear(struct emac_adapter *adpt, u32 irq_bits)
+static int emac_sgmii_irq_clear(struct emac_adapter *adpt, u8 irq_bits)
{
struct emac_sgmii *phy = &adpt->phy;
- u32 status;
+ u8 status;
writel_relaxed(irq_bits, phy->base + EMAC_SGMII_PHY_INTERRUPT_CLEAR);
writel_relaxed(IRQ_GLOBAL_CLEAR, phy->base + EMAC_SGMII_PHY_IRQ_CMD);
@@ -86,9 +86,8 @@ static int emac_sgmii_irq_clear(struct emac_adapter *adpt, u32 irq_bits)
EMAC_SGMII_PHY_INTERRUPT_STATUS,
status, !(status & irq_bits), 1,
SGMII_PHY_IRQ_CLR_WAIT_TIME)) {
- netdev_err(adpt->netdev,
- "error: failed clear SGMII irq: status:0x%x bits:0x%x\n",
- status, irq_bits);
+ net_err_ratelimited("%s: failed to clear SGMII irq: status:0x%x bits:0x%x\n",
+ adpt->netdev->name, status, irq_bits);
return -EIO;
}
@@ -109,7 +108,7 @@ static irqreturn_t emac_sgmii_interrupt(int irq, void *data)
{
struct emac_adapter *adpt = data;
struct emac_sgmii *phy = &adpt->phy;
- u32 status;
+ u8 status;
status = readl(phy->base + EMAC_SGMII_PHY_INTERRUPT_STATUS);
status &= SGMII_ISR_MASK;
@@ -139,10 +138,8 @@ static irqreturn_t emac_sgmii_interrupt(int irq, void *data)
atomic_set(&phy->decode_error_count, 0);
}
- if (emac_sgmii_irq_clear(adpt, status)) {
- netdev_warn(adpt->netdev, "failed to clear SGMII interrupt\n");
+ if (emac_sgmii_irq_clear(adpt, status))
schedule_work(&adpt->work_thread);
- }
return IRQ_HANDLED;
}
diff --git a/drivers/net/ethernet/qualcomm/emac/emac.c b/drivers/net/ethernet/qualcomm/emac/emac.c
index ee6f2d27502c..70c92b649b29 100644
--- a/drivers/net/ethernet/qualcomm/emac/emac.c
+++ b/drivers/net/ethernet/qualcomm/emac/emac.c
@@ -148,9 +148,8 @@ static irqreturn_t emac_isr(int _irq, void *data)
goto exit;
if (status & ISR_ERROR) {
- netif_warn(adpt, intr, adpt->netdev,
- "warning: error irq status 0x%lx\n",
- status & ISR_ERROR);
+ net_err_ratelimited("%s: error interrupt 0x%lx\n",
+ adpt->netdev->name, status & ISR_ERROR);
/* reset MAC */
schedule_work(&adpt->work_thread);
}
@@ -169,7 +168,8 @@ static irqreturn_t emac_isr(int _irq, void *data)
emac_mac_tx_process(adpt, &adpt->tx_q);
if (status & ISR_OVER)
- net_warn_ratelimited("warning: TX/RX overflow\n");
+ net_warn_ratelimited("%s: TX/RX overflow interrupt\n",
+ adpt->netdev->name);
exit:
/* enable the interrupt */
--
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc. Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.
^ permalink raw reply related
* [PATCH 2/4] net: qcom/emac: remove unused address arrays
From: Timur Tabi @ 2017-10-11 19:52 UTC (permalink / raw)
To: David S. Miller, netdev; +Cc: timur
In-Reply-To: <1507751546-10265-1-git-send-email-timur@codeaurora.org>
The EMAC is capable of multiple TX and RX rings, but the driver only
supports one ring for each. One function had some left-over unused
code that supports multiple rings, but all it did was make the code
harder to read.
Signed-off-by: Timur Tabi <timur@codeaurora.org>
---
drivers/net/ethernet/qualcomm/emac/emac-mac.c | 16 +++-------------
1 file changed, 3 insertions(+), 13 deletions(-)
diff --git a/drivers/net/ethernet/qualcomm/emac/emac-mac.c b/drivers/net/ethernet/qualcomm/emac/emac-mac.c
index 3ed9033e56db..9cbb27263742 100644
--- a/drivers/net/ethernet/qualcomm/emac/emac-mac.c
+++ b/drivers/net/ethernet/qualcomm/emac/emac-mac.c
@@ -309,22 +309,12 @@ void emac_mac_mode_config(struct emac_adapter *adpt)
/* Config descriptor rings */
static void emac_mac_dma_rings_config(struct emac_adapter *adpt)
{
- static const unsigned short tpd_q_offset[] = {
- EMAC_DESC_CTRL_8, EMAC_H1TPD_BASE_ADDR_LO,
- EMAC_H2TPD_BASE_ADDR_LO, EMAC_H3TPD_BASE_ADDR_LO};
- static const unsigned short rfd_q_offset[] = {
- EMAC_DESC_CTRL_2, EMAC_DESC_CTRL_10,
- EMAC_DESC_CTRL_12, EMAC_DESC_CTRL_13};
- static const unsigned short rrd_q_offset[] = {
- EMAC_DESC_CTRL_5, EMAC_DESC_CTRL_14,
- EMAC_DESC_CTRL_15, EMAC_DESC_CTRL_16};
-
/* TPD (Transmit Packet Descriptor) */
writel(upper_32_bits(adpt->tx_q.tpd.dma_addr),
adpt->base + EMAC_DESC_CTRL_1);
writel(lower_32_bits(adpt->tx_q.tpd.dma_addr),
- adpt->base + tpd_q_offset[0]);
+ adpt->base + EMAC_DESC_CTRL_8);
writel(adpt->tx_q.tpd.count & TPD_RING_SIZE_BMSK,
adpt->base + EMAC_DESC_CTRL_9);
@@ -334,9 +324,9 @@ static void emac_mac_dma_rings_config(struct emac_adapter *adpt)
adpt->base + EMAC_DESC_CTRL_0);
writel(lower_32_bits(adpt->rx_q.rfd.dma_addr),
- adpt->base + rfd_q_offset[0]);
+ adpt->base + EMAC_DESC_CTRL_2);
writel(lower_32_bits(adpt->rx_q.rrd.dma_addr),
- adpt->base + rrd_q_offset[0]);
+ adpt->base + EMAC_DESC_CTRL_5);
writel(adpt->rx_q.rfd.count & RFD_RING_SIZE_BMSK,
adpt->base + EMAC_DESC_CTRL_3);
--
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc. Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.
^ permalink raw reply related
* [PATCH 0/4] net: qcom/emac: various minor fixes
From: Timur Tabi @ 2017-10-11 19:52 UTC (permalink / raw)
To: David S. Miller, netdev; +Cc: timur
A set of patches for 4.15 that clean up some code, apply minors fixes,
and so on. Some of the code also prepares the driver for a future
version of the EMAC controller.
Timur Tabi (4):
net: qcom/emac: specify the correct DMA mask
net: qcom/emac: remove unused address arrays
net: qcom/emac: enforce DMA address restrictions
net: qcom/emac: clean up some TX/RX error messages
drivers/net/ethernet/qualcomm/emac/emac-mac.c | 55 ++++++++++++-------------
drivers/net/ethernet/qualcomm/emac/emac-sgmii.c | 15 +++----
drivers/net/ethernet/qualcomm/emac/emac.c | 25 ++++-------
3 files changed, 41 insertions(+), 54 deletions(-)
--
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc. Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.
^ permalink raw reply
* Re: [PATCH] [net-next]NFC: Convert timers to use timer_setup()
From: Allen @ 2017-10-11 19:52 UTC (permalink / raw)
To: davem-fT/PcQaiUtIeIZ0/mPfg9Q
Cc: linux-wireless-u79uwXL29TY76Z2rM5mHXA,
netdev-u79uwXL29TY76Z2rM5mHXA, keescook-F7+t8E8rja9g9hUCZPvPmw
In-Reply-To: <1507718024-2134-1-git-send-email-allen.pais-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
> Switch to using the new timer_setup() and from_timer()
> for net/nfc/*
>
> Signed-off-by: Allen Pais <allen.pais-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
> ---
> ---
> net/nfc/core.c | 8 +++-----
> net/nfc/hci/core.c | 7 +++----
> net/nfc/hci/llc_shdlc.c | 23 +++++++++--------------
> net/nfc/llcp_core.c | 14 ++++++--------
> 4 files changed, 21 insertions(+), 31 deletions(-)
>
My bad, I missed the Reviewed-by.
Reviewed-by: Kees Cook <keescook-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
- Allen
^ permalink raw reply
* [PATCH 1/4] net: qcom/emac: specify the correct DMA mask
From: Timur Tabi @ 2017-10-11 19:52 UTC (permalink / raw)
To: David S. Miller, netdev; +Cc: timur
In-Reply-To: <1507751546-10265-1-git-send-email-timur@codeaurora.org>
The 64/32-bit DMA mask hackery in the EMAC driver is not actually necessary,
and is technically not accurate. The EMAC hardware is limted to a 45-bit
DMA address. Although no EMAC-enabled system can have that much DDR,
an IOMMU could possible provide a larger address. Rather than play games
with the DMA mappings, the driver should provide a correct value and
trust the DMA/IOMMU layers to do the right thing.
Signed-off-by: Timur Tabi <timur@codeaurora.org>
---
drivers/net/ethernet/qualcomm/emac/emac.c | 17 ++++-------------
1 file changed, 4 insertions(+), 13 deletions(-)
diff --git a/drivers/net/ethernet/qualcomm/emac/emac.c b/drivers/net/ethernet/qualcomm/emac/emac.c
index f477ba29c569..ee6f2d27502c 100644
--- a/drivers/net/ethernet/qualcomm/emac/emac.c
+++ b/drivers/net/ethernet/qualcomm/emac/emac.c
@@ -615,20 +615,11 @@ static int emac_probe(struct platform_device *pdev)
u32 reg;
int ret;
- /* The EMAC itself is capable of 64-bit DMA, so try that first. */
- ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));
+ /* The TPD buffer address is limited to 45 bits. */
+ ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(45));
if (ret) {
- /* Some platforms may restrict the EMAC's address bus to less
- * then the size of DDR. In this case, we need to try a
- * smaller mask. We could try every possible smaller mask,
- * but that's overkill. Instead, just fall to 32-bit, which
- * should always work.
- */
- ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
- if (ret) {
- dev_err(&pdev->dev, "could not set DMA mask\n");
- return ret;
- }
+ dev_err(&pdev->dev, "could not set DMA mask\n");
+ return ret;
}
netdev = alloc_etherdev(sizeof(struct emac_adapter));
--
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc. Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.
^ permalink raw reply related
* Re: next: arm64: LTP sendto01 test causes system crash in ilp32 mode
From: Yury Norov @ 2017-10-11 19:43 UTC (permalink / raw)
To: Eric Dumazet
Cc: linux-kernel, linux-arm-kernel, netdev, Catalin Marinas,
David S. Miller, Florian Westphal
In-Reply-To: <1507747717.31614.28.camel@edumazet-glaptop3.roam.corp.google.com>
Hi Eric,
On Wed, Oct 11, 2017 at 11:48:37AM -0700, Eric Dumazet wrote:
> On Wed, 2017-10-11 at 11:41 -0700, Eric Dumazet wrote:
> > On Wed, 2017-10-11 at 21:35 +0300, Yury Norov wrote:
> > > Hi all,
> > >
> > > It seems like next-20171009 with ilp32 patches crashes on LTP sendto01 test
> > > in sys_sendto() path, like this:
> >
> > Thanks for the report.
> > Probably caused by one of my recent patches, so I am taking a look.
>
> Yes, this was silly.
>
> Please test this fix :
>
> diff --git a/include/net/tcp.h b/include/net/tcp.h
> index 5a95e5886b55e03e4a8bfeac3506c657a4f97dde..15163454174babdcb465904f725b919268dd1bc7 100644
> --- a/include/net/tcp.h
> +++ b/include/net/tcp.h
> @@ -1712,6 +1712,7 @@ static inline void tcp_insert_write_queue_before(struct sk_buff *new,
>
> static inline void tcp_unlink_write_queue(struct sk_buff *skb, struct sock *sk)
> {
> + tcp_skb_tsorted_anchor_cleanup(skb);
> __skb_unlink(skb, &sk->sk_write_queue);
> }
The fix works for me, thanks.
Tested-by: Yury Norov <ynorov@caviumnetworks.com>
^ 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