* Re: [PATCH] rtlwifi: rtl8821ae: add in a missing break in switch statement
From: Kalle Valo @ 2018-10-06 19:30 UTC (permalink / raw)
To: Colin King
Cc: Ping-Ke Shih, David S . Miller, Larry Finger, Tsang-Shian Lin,
linux-wireless, netdev, kernel-janitors, linux-kernel
In-Reply-To: <20181006184246.29985-1-colin.king@canonical.com>
Colin King <colin.king@canonical.com> writes:
> From: Colin Ian King <colin.king@canonical.com>
>
> The switch case RATR_INX_WIRELESS_MC has a missing break, this seems
> to be unintentional as the setting of variable ret gets overwritten
> when the case falls through to the following RATR_INX_WIRELESS_AC_5N
> case. Fix this by adding in the missing break.
>
> Detected by CoverityScan, CID#1167237 ("Missing break in switch")
>
> Fixes: 3c05bedb5fef ("Staging: rtl8812ae: Add Realtek 8821 PCI WIFI driver")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
> drivers/net/wireless/realtek/rtlwifi/rtl8821ae/hw.c | 1 +
Is the fixes line correct? This patch is not for staging.
--
Kalle Valo
^ permalink raw reply
* Re: [PATCH net-next v7 28/28] net: WireGuard secure network tunnel
From: Eugene Syromiatnikov @ 2018-10-06 19:43 UTC (permalink / raw)
To: Jason A. Donenfeld; +Cc: linux-kernel, netdev, davem, gregkh
In-Reply-To: <20181006025709.4019-29-Jason@zx2c4.com>
On Sat, Oct 06, 2018 at 04:57:09AM +0200, Jason A. Donenfeld wrote:
> +static int get_allowedips(void *ctx, const u8 *ip, u8 cidr, int family)
> +{
> + struct allowedips_ctx *actx = ctx;
> + struct nlattr *allowedip_nest;
> +
> + allowedip_nest = nla_nest_start(actx->skb, actx->i++);
Second parameter of nl_nest_start is an attribute type; (ab)using it as
array index leads to special handling of such structures in parsers.
It's better to have some type like WGDEVICE_A_PEER_ITEM and provide an
additional attribute inside it for index (WGPEER_A_INDEX?).
See, for example, commit v4.12-rc1~119^2~131 ("nbd: add a status netlink
command").
> +static int get_peer(struct wireguard_peer *peer, unsigned int index,
> + struct allowedips_cursor *rt_cursor, struct sk_buff *skb)
> +{
> + struct nlattr *allowedips_nest, *peer_nest = nla_nest_start(skb, index);
Same here.
^ permalink raw reply
* [PATCH] net: sched: pie: fix coding style issues
From: Leslie Monis @ 2018-10-06 19:52 UTC (permalink / raw)
To: jhs; +Cc: netdev, linux-kernel, Leslie Monis
Fix 5 warnings and 14 checks issued by checkpatch.pl:
CHECK: Logical continuations should be on the previous line
+ if ((q->vars.qdelay < q->params.target / 2)
+ && (q->vars.prob < MAX_PROB / 5))
WARNING: line over 80 characters
+ q->params.tupdate = usecs_to_jiffies(nla_get_u32(tb[TCA_PIE_TUPDATE]));
CHECK: Blank lines aren't necessary after an open brace '{'
+{
+
CHECK: braces {} should be used on all arms of this statement
+ if (qlen < QUEUE_THRESHOLD)
[...]
+ else {
[...]
CHECK: Unbalanced braces around else statement
+ else {
CHECK: No space is necessary after a cast
+ if (delta > (s32) (MAX_PROB / (100 / 2)) &&
CHECK: Unnecessary parentheses around 'qdelay == 0'
+ if ((qdelay == 0) && (qdelay_old == 0) && update_prob)
CHECK: Unnecessary parentheses around 'qdelay_old == 0'
+ if ((qdelay == 0) && (qdelay_old == 0) && update_prob)
CHECK: Unnecessary parentheses around 'q->vars.prob == 0'
+ if ((q->vars.qdelay < q->params.target / 2) &&
+ (q->vars.qdelay_old < q->params.target / 2) &&
+ (q->vars.prob == 0) &&
+ (q->vars.avg_dq_rate > 0))
CHECK: Unnecessary parentheses around 'q->vars.avg_dq_rate > 0'
+ if ((q->vars.qdelay < q->params.target / 2) &&
+ (q->vars.qdelay_old < q->params.target / 2) &&
+ (q->vars.prob == 0) &&
+ (q->vars.avg_dq_rate > 0))
CHECK: Blank lines aren't necessary before a close brace '}'
+
+}
CHECK: Comparison to NULL could be written "!opts"
+ if (opts == NULL)
CHECK: No space is necessary after a cast
+ ((u32) PSCHED_TICKS2NS(q->params.target)) /
WARNING: line over 80 characters
+ nla_put_u32(skb, TCA_PIE_TUPDATE, jiffies_to_usecs(q->params.tupdate)) ||
CHECK: Blank lines aren't necessary before a close brace '}'
+
+}
CHECK: No space is necessary after a cast
+ .delay = ((u32) PSCHED_TICKS2NS(q->vars.qdelay)) /
WARNING: Missing a blank line after declarations
+ struct sk_buff *skb;
+ skb = qdisc_dequeue_head(sch);
WARNING: Missing a blank line after declarations
+ struct pie_sched_data *q = qdisc_priv(sch);
+ qdisc_reset_queue(sch);
WARNING: Missing a blank line after declarations
+ struct pie_sched_data *q = qdisc_priv(sch);
+ q->params.tupdate = 0;
Signed-off-by: Leslie Monis <lesliemonis@gmail.com>
---
net/sched/sch_pie.c | 36 ++++++++++++++++++------------------
1 file changed, 18 insertions(+), 18 deletions(-)
diff --git a/net/sched/sch_pie.c b/net/sched/sch_pie.c
index 18d30bb..d142937 100644
--- a/net/sched/sch_pie.c
+++ b/net/sched/sch_pie.c
@@ -110,8 +110,8 @@ static bool drop_early(struct Qdisc *sch, u32 packet_size)
/* If current delay is less than half of target, and
* if drop prob is low already, disable early_drop
*/
- if ((q->vars.qdelay < q->params.target / 2)
- && (q->vars.prob < MAX_PROB / 5))
+ if ((q->vars.qdelay < q->params.target / 2) &&
+ (q->vars.prob < MAX_PROB / 5))
return false;
/* If we have fewer than 2 mtu-sized packets, disable drop_early,
@@ -209,7 +209,8 @@ static int pie_change(struct Qdisc *sch, struct nlattr *opt,
/* tupdate is in jiffies */
if (tb[TCA_PIE_TUPDATE])
- q->params.tupdate = usecs_to_jiffies(nla_get_u32(tb[TCA_PIE_TUPDATE]));
+ q->params.tupdate =
+ usecs_to_jiffies(nla_get_u32(tb[TCA_PIE_TUPDATE]));
if (tb[TCA_PIE_LIMIT]) {
u32 limit = nla_get_u32(tb[TCA_PIE_LIMIT]);
@@ -247,7 +248,6 @@ static int pie_change(struct Qdisc *sch, struct nlattr *opt,
static void pie_process_dequeue(struct Qdisc *sch, struct sk_buff *skb)
{
-
struct pie_sched_data *q = qdisc_priv(sch);
int qlen = sch->qstats.backlog; /* current queue size in bytes */
@@ -294,9 +294,9 @@ static void pie_process_dequeue(struct Qdisc *sch, struct sk_buff *skb)
* dq_count to 0 to re-enter the if block when the next
* packet is dequeued
*/
- if (qlen < QUEUE_THRESHOLD)
+ if (qlen < QUEUE_THRESHOLD) {
q->vars.dq_count = DQCOUNT_INVALID;
- else {
+ } else {
q->vars.dq_count = 0;
q->vars.dq_tstamp = psched_get_time();
}
@@ -370,7 +370,7 @@ static void calculate_probability(struct Qdisc *sch)
oldprob = q->vars.prob;
/* to ensure we increase probability in steps of no more than 2% */
- if (delta > (s32) (MAX_PROB / (100 / 2)) &&
+ if (delta > (s32)(MAX_PROB / (100 / 2)) &&
q->vars.prob >= MAX_PROB / 10)
delta = (MAX_PROB / 100) * 2;
@@ -405,7 +405,7 @@ static void calculate_probability(struct Qdisc *sch)
* delay is 0 for 2 consecutive Tupdate periods.
*/
- if ((qdelay == 0) && (qdelay_old == 0) && update_prob)
+ if (qdelay == 0 && qdelay_old == 0 && update_prob)
q->vars.prob = (q->vars.prob * 98) / 100;
q->vars.qdelay = qdelay;
@@ -419,8 +419,8 @@ static void calculate_probability(struct Qdisc *sch)
*/
if ((q->vars.qdelay < q->params.target / 2) &&
(q->vars.qdelay_old < q->params.target / 2) &&
- (q->vars.prob == 0) &&
- (q->vars.avg_dq_rate > 0))
+ q->vars.prob == 0 &&
+ q->vars.avg_dq_rate > 0)
pie_vars_init(&q->vars);
}
@@ -437,7 +437,6 @@ static void pie_timer(struct timer_list *t)
if (q->params.tupdate)
mod_timer(&q->adapt_timer, jiffies + q->params.tupdate);
spin_unlock(root_lock);
-
}
static int pie_init(struct Qdisc *sch, struct nlattr *opt,
@@ -469,15 +468,16 @@ static int pie_dump(struct Qdisc *sch, struct sk_buff *skb)
struct nlattr *opts;
opts = nla_nest_start(skb, TCA_OPTIONS);
- if (opts == NULL)
+ if (!opts)
goto nla_put_failure;
/* convert target from pschedtime to us */
if (nla_put_u32(skb, TCA_PIE_TARGET,
- ((u32) PSCHED_TICKS2NS(q->params.target)) /
+ ((u32)PSCHED_TICKS2NS(q->params.target)) /
NSEC_PER_USEC) ||
nla_put_u32(skb, TCA_PIE_LIMIT, sch->limit) ||
- nla_put_u32(skb, TCA_PIE_TUPDATE, jiffies_to_usecs(q->params.tupdate)) ||
+ nla_put_u32(skb, TCA_PIE_TUPDATE,
+ jiffies_to_usecs(q->params.tupdate)) ||
nla_put_u32(skb, TCA_PIE_ALPHA, q->params.alpha) ||
nla_put_u32(skb, TCA_PIE_BETA, q->params.beta) ||
nla_put_u32(skb, TCA_PIE_ECN, q->params.ecn) ||
@@ -489,7 +489,6 @@ static int pie_dump(struct Qdisc *sch, struct sk_buff *skb)
nla_put_failure:
nla_nest_cancel(skb, opts);
return -1;
-
}
static int pie_dump_stats(struct Qdisc *sch, struct gnet_dump *d)
@@ -497,7 +496,7 @@ static int pie_dump_stats(struct Qdisc *sch, struct gnet_dump *d)
struct pie_sched_data *q = qdisc_priv(sch);
struct tc_pie_xstats st = {
.prob = q->vars.prob,
- .delay = ((u32) PSCHED_TICKS2NS(q->vars.qdelay)) /
+ .delay = ((u32)PSCHED_TICKS2NS(q->vars.qdelay)) /
NSEC_PER_USEC,
/* unscale and return dq_rate in bytes per sec */
.avg_dq_rate = q->vars.avg_dq_rate *
@@ -514,8 +513,7 @@ static int pie_dump_stats(struct Qdisc *sch, struct gnet_dump *d)
static struct sk_buff *pie_qdisc_dequeue(struct Qdisc *sch)
{
- struct sk_buff *skb;
- skb = qdisc_dequeue_head(sch);
+ struct sk_buff *skb = qdisc_dequeue_head(sch);
if (!skb)
return NULL;
@@ -527,6 +525,7 @@ static struct sk_buff *pie_qdisc_dequeue(struct Qdisc *sch)
static void pie_reset(struct Qdisc *sch)
{
struct pie_sched_data *q = qdisc_priv(sch);
+
qdisc_reset_queue(sch);
pie_vars_init(&q->vars);
}
@@ -534,6 +533,7 @@ static void pie_reset(struct Qdisc *sch)
static void pie_destroy(struct Qdisc *sch)
{
struct pie_sched_data *q = qdisc_priv(sch);
+
q->params.tupdate = 0;
del_timer_sync(&q->adapt_timer);
}
--
2.7.4
^ permalink raw reply related
* Re: [PATCH] wil6210: fix debugfs_simple_attr.cocci warnings
From: Kalle Valo @ 2018-10-06 12:54 UTC (permalink / raw)
To: Julia Lawall
Cc: YueHaibing, Maya Erez, linux-wireless, wil6210, kernel-janitors,
netdev
In-Reply-To: <alpine.DEB.2.21.1810061422030.2363@hadrien>
Julia Lawall <julia.lawall@lip6.fr> writes:
> On Sat, 6 Oct 2018, Kalle Valo wrote:
>
>> Julia Lawall <julia.lawall@lip6.fr> writes:
>>
>> > On Fri, 5 Oct 2018, Kalle Valo wrote:
>> >
>> >> YueHaibing <yuehaibing@huawei.com> writes:
>> >>
>> >> > Use DEFINE_DEBUGFS_ATTRIBUTE rather than DEFINE_SIMPLE_ATTRIBUTE
>> >> > for debugfs files.
>> >> >
>> >> > Semantic patch information:
>> >> > Rationale: DEFINE_SIMPLE_ATTRIBUTE + debugfs_create_file()
>> >> > imposes some significant overhead as compared to
>> >> > DEFINE_DEBUGFS_ATTRIBUTE + debugfs_create_file_unsafe().
>> >> >
>> >> > Generated by: scripts/coccinelle/api/debugfs/debugfs_simple_attr.cocci
>> >>
>> >> Just out of curiosity, what kind of overhead are we talking about here?
>> >
>> > The log message on the commit introducing the semantic patch says the
>> > following:
>> >
>> > In order to protect against file removal races, debugfs files created via
>> > debugfs_create_file() now get wrapped by a struct file_operations at their
>> > opening.
>> >
>> > If the original struct file_operations are known to be safe against removal
>> > races by themselves already, the proxy creation may be bypassed by creating
>> > the files through debugfs_create_file_unsafe().
>> >
>> > In order to help debugfs users who use the common
>> > DEFINE_SIMPLE_ATTRIBUTE() + debugfs_create_file()
>> > idiom to transition to removal safe struct file_operations, the helper
>> > macro DEFINE_DEBUGFS_ATTRIBUTE() has been introduced.
>> >
>> > Thus, the preferred strategy is to use
>> > DEFINE_DEBUGFS_ATTRIBUTE() + debugfs_create_file_unsafe()
>> > now.
>>
>> I admit that I didn't have time to investigate this is detail but I'm
>> still not understanding where is that "significant overhead" coming from
>> and how big of overhead are we talking about? I guess it has something
>> to do with full_proxy_open() vs open_proxy_open()?
>>
>> Not that I'm against this patch, just curious when I see someone
>> claiming "significant overhead" which is not obvious for me.
>
> The message with the semantic patch doesn't really talk about significant
> overhead. Maybe YueHaibing can discuss with the person who proposed the
> semantic patch what the actual issue is, and when the proposed change is
> actually applicable.
Actually commit 5103068eaca2 mentions "significant overhead":
--- /dev/null
+++ b/scripts/coccinelle/api/debugfs/debugfs_simple_attr.cocci
@@ -0,0 +1,67 @@
+/// Use DEFINE_DEBUGFS_ATTRIBUTE rather than DEFINE_SIMPLE_ATTRIBUTE
+/// for debugfs files.
+///
+//# Rationale: DEFINE_SIMPLE_ATTRIBUTE + debugfs_create_file()
+//# imposes some significant overhead as compared to
+//# DEFINE_DEBUGFS_ATTRIBUTE + debugfs_create_file_unsafe().
But I'll anyway apply this patch as I don't see anything wrong with it.
I was just trying to learn where this overhead is :)
--
Kalle Valo
^ permalink raw reply
* [PATCH] libertas: don't set URB_ZERO_PACKET on IN USB transfer
From: Lubomir Rintel @ 2018-10-06 20:12 UTC (permalink / raw)
To: Kalle Valo
Cc: David S. Miller, libertas-dev, linux-wireless, netdev,
linux-kernel, Lubomir Rintel, stable
The USB core gets rightfully upset:
usb 1-1: BOGUS urb flags, 240 --> 200
WARNING: CPU: 0 PID: 60 at drivers/usb/core/urb.c:503 usb_submit_urb+0x2f8/0x3ed
Modules linked in:
CPU: 0 PID: 60 Comm: kworker/0:3 Not tainted 4.19.0-rc6-00319-g5206d00a45c7 #39
Hardware name: OLPC XO/XO, BIOS OLPC Ver 1.00.01 06/11/2014
Workqueue: events request_firmware_work_func
EIP: usb_submit_urb+0x2f8/0x3ed
Code: 75 06 8b 8f 80 00 00 00 8d 47 78 89 4d e4 89 55 e8 e8 35 1c f6 ff 8b 55 e8 56 52 8b 4d e4 51 50 68 e3 ce c7 c0 e8 ed 18 c6 ff <0f> 0b 83 c4 14 80 7d ef 01 74 0a 80 7d ef 03 0f 85 b8 00 00 00 8b
EAX: 00000025 EBX: ce7d4980 ECX: 00000000 EDX: 00000001
ESI: 00000200 EDI: ce7d8800 EBP: ce7f5ea8 ESP: ce7f5e70
DS: 007b ES: 007b FS: 0000 GS: 00e0 SS: 0068 EFLAGS: 00210292
CR0: 80050033 CR2: 00000000 CR3: 00e80000 CR4: 00000090
Call Trace:
? if_usb_fw_timeo+0x64/0x64
__if_usb_submit_rx_urb+0x85/0xe6
? if_usb_fw_timeo+0x64/0x64
if_usb_submit_rx_urb_fwload+0xd/0xf
if_usb_prog_firmware+0xc0/0x3db
? _request_firmware+0x54/0x47b
? _request_firmware+0x89/0x47b
? if_usb_probe+0x412/0x412
lbs_fw_loaded+0x55/0xa6
? debug_smp_processor_id+0x12/0x14
helper_firmware_cb+0x3c/0x3f
request_firmware_work_func+0x37/0x6f
process_one_work+0x164/0x25a
worker_thread+0x1c4/0x284
kthread+0xec/0xf1
? cancel_delayed_work_sync+0xf/0xf
? kthread_create_on_node+0x1a/0x1a
ret_from_fork+0x2e/0x38
---[ end trace 3ef1e3b2dd53852f ]---
Cc: stable@vger.kernel.org
Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
---
drivers/net/wireless/marvell/libertas/if_usb.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/net/wireless/marvell/libertas/if_usb.c b/drivers/net/wireless/marvell/libertas/if_usb.c
index 5fee555a3d60..220dcdee8d2b 100644
--- a/drivers/net/wireless/marvell/libertas/if_usb.c
+++ b/drivers/net/wireless/marvell/libertas/if_usb.c
@@ -459,8 +459,6 @@ static int __if_usb_submit_rx_urb(struct if_usb_card *cardp,
MRVDRV_ETH_RX_PACKET_BUFFER_SIZE, callbackfn,
cardp);
- cardp->rx_urb->transfer_flags |= URB_ZERO_PACKET;
-
lbs_deb_usb2(&cardp->udev->dev, "Pointer for rx_urb %p\n", cardp->rx_urb);
if ((ret = usb_submit_urb(cardp->rx_urb, GFP_ATOMIC))) {
lbs_deb_usbd(&cardp->udev->dev, "Submit Rx URB failed: %d\n", ret);
--
2.19.0
^ permalink raw reply related
* Re: [PATCH] rtlwifi: rtl8821ae: add in a missing break in switch statement
From: Joe Perches @ 2018-10-06 20:17 UTC (permalink / raw)
To: Larry Finger, Kalle Valo, Colin King
Cc: Ping-Ke Shih, David S . Miller, Tsang-Shian Lin, linux-wireless,
netdev, kernel-janitors, linux-kernel
In-Reply-To: <a5113f03-7ac8-4be6-3d1c-5ba74639121e@lwfinger.net>
On Sat, 2018-10-06 at 15:05 -0500, Larry Finger wrote:
> On 10/6/18 2:30 PM, Kalle Valo wrote:
> > Colin King <colin.king@canonical.com> writes:
> >
> > > From: Colin Ian King <colin.king@canonical.com>
> > >
> > > The switch case RATR_INX_WIRELESS_MC has a missing break, this seems
> > > to be unintentional as the setting of variable ret gets overwritten
> > > when the case falls through to the following RATR_INX_WIRELESS_AC_5N
> > > case. Fix this by adding in the missing break.
> > >
> > > Detected by CoverityScan, CID#1167237 ("Missing break in switch")
> > >
> > > Fixes: 3c05bedb5fef ("Staging: rtl8812ae: Add Realtek 8821 PCI WIFI driver")
> > > Signed-off-by: Colin Ian King <colin.king@canonical.com>
> > > ---
> > > drivers/net/wireless/realtek/rtlwifi/rtl8821ae/hw.c | 1 +
> >
> > Is the fixes line correct? This patch is not for staging.
>
> No, the correct fixes commit is 21e4b0726dc67 (" rtlwifi: rtl8821ae: Move driver
> from staging to regular tree").
>
> This driver was initially placed in staging as it was needed for a special
> project, which is the commit that Colin used. As the patch subject states, the
> driver was later moved to the regular wireless tree.
>
> That break is required, thus ACKed-by: Larry Finger <Larry.Finger@lwfinger.net>
Why not remove this entirely and use the generic routine in
drivers/net/wireless/realtek/rtlwifi/base.c?
Is there a real difference?
^ permalink raw reply
* Re: [PATCH] team: set IFF_SLAVE on team ports
From: Chas Williams @ 2018-10-06 13:28 UTC (permalink / raw)
To: Jiri Pirko; +Cc: Stephen Hemminger, Jan Blunck, LKML, netdev
In-Reply-To: <20181005064637.GA3061@nanopsycho.orion>
On 10/05/18 02:46, Jiri Pirko wrote:
> Wed, Oct 03, 2018 at 07:30:06PM CEST, 3chas3@gmail.com wrote:
>>
>>
>> On 10/03/18 06:44, Jiri Pirko wrote:
>>> Tue, Oct 02, 2018 at 11:20:25PM CEST, 3chas3@gmail.com wrote:
>>>>
>>>>
>>>> On 10/02/18 07:12, Jiri Pirko wrote:
>>>>> Mon, Oct 01, 2018 at 04:06:16PM CEST, 3chas3@gmail.com wrote:
>>>>>>
>>>>>>
>>>>>> On 09/30/18 05:34, Jiri Pirko wrote:
>>>>>>> Sun, Sep 30, 2018 at 11:38:05AM CEST, stephen@networkplumber.org wrote:
>>>>>>>> On Sun, 30 Sep 2018 09:14:14 +0200
>>>>>>>> Jiri Pirko <jiri@resnulli.us> wrote:
>>>>>>>>
>>>>>>>>> Thu, Sep 27, 2018 at 04:04:26PM CEST, 3chas3@gmail.com wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On 07/10/15 02:41, Jiri Pirko wrote:
>>>>>>>>>>> Thu, Jul 09, 2015 at 05:36:55PM CEST, jblunck@infradead.org wrote:
>>>>>>>>>>>> On Thu, Jul 9, 2015 at 12:07 PM, Jiri Pirko <jiri@resnulli.us> wrote:
>>>>>>>>>>>>> Thu, Jul 09, 2015 at 11:58:34AM CEST, jblunck@infradead.org wrote:
>>>>>>>>>>>>>> The code in net/ipv6/addrconf.c:addrconf_notify() tests for IFF_SLAVE to
>>>>>>>>>>>>>> decide if it should start the address configuration. Since team ports
>>>>>>>>>>>>>> shouldn't get link-local addresses assigned lets set IFF_SLAVE when linking
>>>>>>>>>>>>>> a port to the team master.
>>>>>>>>>>>>>
>>>>>>>>>>>>> I don't want to use IFF_SLAVE in team. Other master-slave devices are
>>>>>>>>>>>>> not using that as well, for example bridge, ovs, etc.
>>>>>>>>>>>>
>>>>>>>>>>>> Maybe they need to get fixed too. I've used that flag because it is
>>>>>>>>>>>> documented as
>>>>>>>>>>>> a "slave of a load balancer" which describes what a team port is.
>>>>>>>>>>>>
>>>>>>>>>>>>> I think that this should be fixed in addrconf_notify. It should lookup
>>>>>>>>>>>>> if there is a master on top and bail out in that case.
>>>>>>>>>>>>
>>>>>>>>>>>> There are other virtual interfaces that have a master assigned and want to
>>>>>>>>>>>> participate in IPv6 address configuration.
>>>>>>>>>>>
>>>>>>>>>>> Can you give me an example?
>>>>>>>>>>
>>>>>>>>>> I would like to revisit this patch (yes, I know it has been a while). I
>>>>>>>>>> believe the VRF implementation uses master to group the interfaces under
>>>>>>>>>> a single interface.
>>>>>>>>>>
>>>>>>>>>> I don't see a reason not to use IFF_SLAVE since team and bonding are fairly
>>>>>>>>>> similar.
>>>>>>>>>
>>>>>>>>> Again, why do you need team port to have IFF_SLAVE flag? What do you
>>>>>>>>> want to achieve
>>>>>>>>
>>>>>>>> Without setting this flag IPv6 will try and make a link specific address.
>>>>>
>>>>> You are talking about addrconf_notify() right? Easy to fix to check
>>>>> something more convenient. Like netif_is_lag_port() if you want to avoid
>>>>> it for bond/team. netif_is_ovs_port(), netif_is_bridge_port() etc. Lot's
>>>>> of helpers to cover this.
>>>>
>>>> OK, IPv6 should probably be using this.
>>>>
>>>>>
>>>>>
>>>>>
>>>>>>>
>>>>>>> Why is it not an issue with bridge, ovs, and other master-slave devices?
>>>>>>>
>>>>>>
>>>>>> It very well might be an issue for bridge and ovs. Other master-slave
>>>>>> devices include the existing VRF implementation in the kernel and those slave
>>>>>> interfaces will certainly want to use IPv6.
>>>>>>
>>>>>> However, IFF_SLAVE has a specific meaning:
>>>>>>
>>>>>> ./include/uapi/linux/if.h: * @IFF_SLAVE: slave of a load balancer. Volatile.
>>>>>
>>>>> I know that some userspace apps are using this flag to determine a
>>>>> "bonding slave". I don't think that they care much about eql...
>>>>>
>>>>>
>>>>>>
>>>>>> The bonding driver is not the only user:
>>>>>>
>>>>>> ./drivers/net/eql.c:#define eql_is_slave(dev) ((dev->flags & IFF_SLAVE) ==
>>>>>> IFF_SLAVE)
>>>>>> ./drivers/net/eql.c: slave->dev->flags &= ~IFF_SLAVE;
>>>>>> ./drivers/net/eql.c: slave->dev->flags |= IFF_SLAVE;
>>>>>>
>>>>>> The team driver would like to use this same flag since it is a load balancer
>>>>>> as well. The side effect of not assigning IPv6 is a bonus. The fact that
>>>>>
>>>>> No, please leave IFF_SLAVE as it is. Both kernel and userspace have
>>>>> their clear indications right now about the master/slave relationships.
>>>>
>>>> The team driver does create a master/slave relationship. The team slaves are
>>>> literally slaves of the master device. It's not clear to me
>>>> why you we can't mark the slaves of the team master as actually being
>>>> slave interfaces?
>>>
>>> So? IFF_SLAVE flag serves a different purpose. That's it. Team does not
>>> need it, bridge does not need it, macvlan does not need it, etc.
>>
>> I agree. But team *is* a load balancer. Why can't team mark its slave
>> interfaces as IFF_SLAVE? They are literally slaves of a load balancer which
>> is the exact meaning of the IFF_SLAVE flag.
>
> I described that multiple times, don't want to repeat myself. Please
> read the thread again.
I have read the flag and you never described what the flag is for. The
only vague mention is "to indicate a bonding slave". A team slave is
exactly the same thing as a bonding slave. If there is some application
using IFF_SLAVE to find those slaves, it should worry about team slaves
as well. Given that the eql driver is using this flag for the same
purpose doesn't give bonding exclusive rights to use this flag with its
interfaces.
>
>>
>>>
>>>
>>>>
>>>>>
>>>>>
>>>>>> bridges and ovs are also likely broken is a different issue. Should there be
>>>>>> a another flag that says "layer 2 only"? Very possibly, but that is
>>>>>> something all these interfaces should be using to include bonding, team, eql,
>>>>>> obs, bridge etc. That's not a reasonable objection to labeling the team
>>>>>> slave as slaves since they are literally slaves of a load balancer.
>>>>>>
>>>>>>
>>>>>>
^ permalink raw reply
* [PATCH net 1/2] net/smc: use __aigned_u64 for 64-bit smc_diag fields
From: Eugene Syromiatnikov @ 2018-10-06 20:32 UTC (permalink / raw)
To: netdev
Cc: David S. Miller, linux-kernel, Ursula Braun, Karsten Graul,
Hans Wippel
Commit v4.19-rc1~140^2~507^2~1 ("net/smc: add SMC-D diag support")
introduced new UAPI-exposed structure, struct smcd_diag_dmbinfo.
However, it's not usable by compat binaries, as it has different
layout there. Probably, the most straightforward fix that will avoid
the similar issues in the future is to use __aligned_u64 for 64-bit
fields.
Fixes: 4b1b7d3b30a6 ("net/smc: add SMC-D diag support")
Signed-off-by: Eugene Syromiatnikov <esyr@redhat.com>
---
include/uapi/linux/smc_diag.h | 22 +++++++++++-----------
1 file changed, 11 insertions(+), 11 deletions(-)
diff --git a/include/uapi/linux/smc_diag.h b/include/uapi/linux/smc_diag.h
index ac9e8c9..6180c6d 100644
--- a/include/uapi/linux/smc_diag.h
+++ b/include/uapi/linux/smc_diag.h
@@ -18,14 +18,14 @@ struct smc_diag_req {
* on the internal clcsock, and more SMC-related socket data
*/
struct smc_diag_msg {
- __u8 diag_family;
- __u8 diag_state;
- __u8 diag_mode;
- __u8 diag_shutdown;
+ __u8 diag_family;
+ __u8 diag_state;
+ __u8 diag_mode;
+ __u8 diag_shutdown;
struct inet_diag_sockid id;
- __u32 diag_uid;
- __u64 diag_inode;
+ __u32 diag_uid;
+ __aligned_u64 diag_inode;
};
/* Mode of a connection */
@@ -99,11 +99,11 @@ struct smc_diag_fallback {
};
struct smcd_diag_dmbinfo { /* SMC-D Socket internals */
- __u32 linkid; /* Link identifier */
- __u64 peer_gid; /* Peer GID */
- __u64 my_gid; /* My GID */
- __u64 token; /* Token of DMB */
- __u64 peer_token; /* Token of remote DMBE */
+ __u32 linkid; /* Link identifier */
+ __aligned_u64 peer_gid; /* Peer GID */
+ __aligned_u64 my_gid; /* My GID */
+ __aligned_u64 token; /* Token of DMB */
+ __aligned_u64 peer_token; /* Token of remote DMBE */
};
#endif /* _UAPI_SMC_DIAG_H_ */
--
2.1.4
^ permalink raw reply related
* [PATCH net 2/2] net/smc: retain old name for diag_mode field
From: Eugene Syromiatnikov @ 2018-10-06 20:32 UTC (permalink / raw)
To: netdev
Cc: David S. Miller, linux-kernel, Ursula Braun, Karsten Graul,
Hans Wippel
Commit v4.19-rc1~140^2~285^2~4 changed the name of diag_fallback field
of struct smc_diag_msg structure to diag_mode. However, this structure
is a part of UAPI, and this change breaks user space applications that
use it ([1], for example). Since the new name is more suitable, convert
the field to a union that provides access to the data via both the new
and the old name.
[1] https://gitlab.com/strace/strace/blob/v4.24/netlink_smc_diag.c#L165
Fixes: c601171d7a60 ("net/smc: provide smc mode in smc_diag.c")
Signed-off-by: Eugene Syromiatnikov <esyr@redhat.com>
---
include/uapi/linux/smc_diag.h | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/include/uapi/linux/smc_diag.h b/include/uapi/linux/smc_diag.h
index 6180c6d..8cb3a6f 100644
--- a/include/uapi/linux/smc_diag.h
+++ b/include/uapi/linux/smc_diag.h
@@ -20,7 +20,10 @@ struct smc_diag_req {
struct smc_diag_msg {
__u8 diag_family;
__u8 diag_state;
- __u8 diag_mode;
+ union {
+ __u8 diag_mode;
+ __u8 diag_fallback; /* the old name of the field */
+ };
__u8 diag_shutdown;
struct inet_diag_sockid id;
--
2.1.4
^ permalink raw reply related
* Re: [PATCH] rtlwifi: rtl8821ae: add in a missing break in switch statement
From: Larry Finger @ 2018-10-06 22:00 UTC (permalink / raw)
To: Joe Perches, Kalle Valo, Colin King
Cc: Ping-Ke Shih, David S . Miller, Tsang-Shian Lin, linux-wireless,
netdev, kernel-janitors, linux-kernel
In-Reply-To: <4e376919169bad265dc02040ae02548d2ac6c503.camel@perches.com>
On 10/6/18 3:17 PM, Joe Perches wrote:
> On Sat, 2018-10-06 at 15:05 -0500, Larry Finger wrote:
>> On 10/6/18 2:30 PM, Kalle Valo wrote:
>>> Colin King <colin.king@canonical.com> writes:
>>>
>>>> From: Colin Ian King <colin.king@canonical.com>
>>>>
>>>> The switch case RATR_INX_WIRELESS_MC has a missing break, this seems
>>>> to be unintentional as the setting of variable ret gets overwritten
>>>> when the case falls through to the following RATR_INX_WIRELESS_AC_5N
>>>> case. Fix this by adding in the missing break.
>>>>
>>>> Detected by CoverityScan, CID#1167237 ("Missing break in switch")
>>>>
>>>> Fixes: 3c05bedb5fef ("Staging: rtl8812ae: Add Realtek 8821 PCI WIFI driver")
>>>> Signed-off-by: Colin Ian King <colin.king@canonical.com>
>>>> ---
>>>> drivers/net/wireless/realtek/rtlwifi/rtl8821ae/hw.c | 1 +
>>>
>>> Is the fixes line correct? This patch is not for staging.
>>
>> No, the correct fixes commit is 21e4b0726dc67 (" rtlwifi: rtl8821ae: Move driver
>> from staging to regular tree").
>>
>> This driver was initially placed in staging as it was needed for a special
>> project, which is the commit that Colin used. As the patch subject states, the
>> driver was later moved to the regular wireless tree.
>>
>> That break is required, thus ACKed-by: Larry Finger <Larry.Finger@lwfinger.net>
>
> Why not remove this entirely and use the generic routine in
> drivers/net/wireless/realtek/rtlwifi/base.c?
>
> Is there a real difference?
I did not see any difference other than the removal of a bunch of magic numbers
and better formatting.
Larry
^ permalink raw reply
* Re: [PATCH] rtlwifi: rtl8821ae: add in a missing break in switch statement
From: Joe Perches @ 2018-10-06 22:03 UTC (permalink / raw)
To: Larry Finger, Kalle Valo, Colin King
Cc: Ping-Ke Shih, David S . Miller, Tsang-Shian Lin, linux-wireless,
netdev, kernel-janitors, linux-kernel
In-Reply-To: <a4286648-dda5-eb46-97bf-f0314ec2d113@lwfinger.net>
On Sat, 2018-10-06 at 17:00 -0500, Larry Finger wrote:
> On 10/6/18 3:17 PM, Joe Perches wrote:
> > On Sat, 2018-10-06 at 15:05 -0500, Larry Finger wrote:
> > > On 10/6/18 2:30 PM, Kalle Valo wrote:
> > > > Colin King <colin.king@canonical.com> writes:
> > > >
> > > > > From: Colin Ian King <colin.king@canonical.com>
> > > > >
> > > > > The switch case RATR_INX_WIRELESS_MC has a missing break, this seems
> > > > > to be unintentional as the setting of variable ret gets overwritten
> > > > > when the case falls through to the following RATR_INX_WIRELESS_AC_5N
> > > > > case. Fix this by adding in the missing break.
> > > > >
> > > > > Detected by CoverityScan, CID#1167237 ("Missing break in switch")
> > > > >
> > > > > Fixes: 3c05bedb5fef ("Staging: rtl8812ae: Add Realtek 8821 PCI WIFI driver")
> > > > > Signed-off-by: Colin Ian King <colin.king@canonical.com>
> > > > > ---
> > > > > drivers/net/wireless/realtek/rtlwifi/rtl8821ae/hw.c | 1 +
> > > >
> > > > Is the fixes line correct? This patch is not for staging.
> > >
> > > No, the correct fixes commit is 21e4b0726dc67 (" rtlwifi: rtl8821ae: Move driver
> > > from staging to regular tree").
> > >
> > > This driver was initially placed in staging as it was needed for a special
> > > project, which is the commit that Colin used. As the patch subject states, the
> > > driver was later moved to the regular wireless tree.
> > >
> > > That break is required, thus ACKed-by: Larry Finger <Larry.Finger@lwfinger.net>
> >
> > Why not remove this entirely and use the generic routine in
> > drivers/net/wireless/realtek/rtlwifi/base.c?
> >
> > Is there a real difference?
>
> I did not see any difference other than the removal of a bunch of magic numbers
> and better formatting.
Me neither.
^ permalink raw reply
* Re: KASAN: use-after-free Read in vhost_transport_cancel_pkt
From: syzbot @ 2018-10-06 15:47 UTC (permalink / raw)
To: jasowang, kvm, linux-kernel, mst, netdev, stefanha,
syzkaller-bugs, virtualization
In-Reply-To: <00000000000068750f0576c178ab@google.com>
syzbot has found a reproducer for the following crash on:
HEAD commit: 091a1eaa0e30 Merge branch 'akpm'
git tree: upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=15627ae6400000
kernel config: https://syzkaller.appspot.com/x/.config?x=c0af03fe452b65fb
dashboard link: https://syzkaller.appspot.com/bug?extid=e3e074963495f92a89ed
compiler: gcc (GCC) 8.0.1 20180413 (experimental)
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=120d5491400000
IMPORTANT: if you fix the bug, please add the following tag to the commit:
Reported-by: syzbot+e3e074963495f92a89ed@syzkaller.appspotmail.com
8021q: adding VLAN 0 to HW filter on device team0
8021q: adding VLAN 0 to HW filter on device team0
8021q: adding VLAN 0 to HW filter on device team0
8021q: adding VLAN 0 to HW filter on device team0
==================================================================
BUG: KASAN: use-after-free in debug_spin_lock_before
kernel/locking/spinlock_debug.c:83 [inline]
BUG: KASAN: use-after-free in do_raw_spin_lock+0x1c0/0x200
kernel/locking/spinlock_debug.c:112
Read of size 4 at addr ffff8801c5e6977c by task syz-executor0/7470
CPU: 1 PID: 7470 Comm: syz-executor0 Not tainted 4.19.0-rc6+ #47
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
Google 01/01/2011
Call Trace:
__dump_stack lib/dump_stack.c:77 [inline]
dump_stack+0x1c4/0x2b4 lib/dump_stack.c:113
print_address_description.cold.8+0x9/0x1ff mm/kasan/report.c:256
kasan_report_error mm/kasan/report.c:354 [inline]
kasan_report.cold.9+0x242/0x309 mm/kasan/report.c:412
__asan_report_load4_noabort+0x14/0x20 mm/kasan/report.c:432
debug_spin_lock_before kernel/locking/spinlock_debug.c:83 [inline]
do_raw_spin_lock+0x1c0/0x200 kernel/locking/spinlock_debug.c:112
__raw_spin_lock_bh include/linux/spinlock_api_smp.h:136 [inline]
_raw_spin_lock_bh+0x39/0x40 kernel/locking/spinlock.c:168
spin_lock_bh include/linux/spinlock.h:334 [inline]
vhost_transport_cancel_pkt+0x15e/0x910 drivers/vhost/vsock.c:244
vsock_transport_cancel_pkt net/vmw_vsock/af_vsock.c:1114 [inline]
vsock_stream_connect+0x903/0xe40 net/vmw_vsock/af_vsock.c:1241
__sys_connect+0x37d/0x4c0 net/socket.c:1664
__do_sys_connect net/socket.c:1675 [inline]
__se_sys_connect net/socket.c:1672 [inline]
__x64_sys_connect+0x73/0xb0 net/socket.c:1672
do_syscall_64+0x1b9/0x820 arch/x86/entry/common.c:290
entry_SYSCALL_64_after_hwframe+0x49/0xbe
RIP: 0033:0x457579
Code: 1d b4 fb ff c3 66 2e 0f 1f 84 00 00 00 00 00 66 90 48 89 f8 48 89 f7
48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff
ff 0f 83 eb b3 fb ff c3 66 2e 0f 1f 84 00 00 00 00
RSP: 002b:00007fbd405dac78 EFLAGS: 00000246 ORIG_RAX: 000000000000002a
RAX: ffffffffffffffda RBX: 0000000000000003 RCX: 0000000000457579
RDX: 0000000000000080 RSI: 0000000020000400 RDI: 0000000000000007
RBP: 000000000072bfa0 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: 00007fbd405db6d4
R13: 00000000004bda00 R14: 00000000004cc478 R15: 00000000ffffffff
Allocated by task 7456:
save_stack+0x43/0xd0 mm/kasan/kasan.c:448
set_track mm/kasan/kasan.c:460 [inline]
kasan_kmalloc+0xc7/0xe0 mm/kasan/kasan.c:553
__do_kmalloc_node mm/slab.c:3682 [inline]
__kmalloc_node+0x47/0x70 mm/slab.c:3689
kmalloc_node include/linux/slab.h:555 [inline]
kvmalloc_node+0xb9/0xf0 mm/util.c:423
kvmalloc include/linux/mm.h:577 [inline]
vhost_vsock_dev_open+0xa2/0x5a0 drivers/vhost/vsock.c:511
misc_open+0x3ca/0x560 drivers/char/misc.c:141
chrdev_open+0x25a/0x710 fs/char_dev.c:417
do_dentry_open+0x499/0x1250 fs/open.c:771
vfs_open+0xa0/0xd0 fs/open.c:880
do_last fs/namei.c:3418 [inline]
path_openat+0x12bf/0x5160 fs/namei.c:3534
^ permalink raw reply
* [PATCH] netfilter: xt_quota: Don't use aligned attribute in sizeof
From: Nathan Chancellor @ 2018-10-06 23:33 UTC (permalink / raw)
To: Pablo Neira Ayuso, Jozsef Kadlecsik, Florian Westphal
Cc: netfilter-devel, coreteam, netdev, linux-kernel,
Nathan Chancellor
Clang warns:
net/netfilter/xt_quota.c:47:44: warning: 'aligned' attribute ignored
when parsing type [-Wignored-attributes]
BUILD_BUG_ON(sizeof(atomic64_t) != sizeof(__aligned_u64));
^~~~~~~~~~~~~
Use 'sizeof(__u64)' instead, as the alignment doesn't affect the size
of the type.
Fixes: e9837e55b020 ("netfilter: xt_quota: fix the behavior of xt_quota module")
Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
---
net/netfilter/xt_quota.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/netfilter/xt_quota.c b/net/netfilter/xt_quota.c
index 6afa7f468a73..fceae245eb03 100644
--- a/net/netfilter/xt_quota.c
+++ b/net/netfilter/xt_quota.c
@@ -44,7 +44,7 @@ static int quota_mt_check(const struct xt_mtchk_param *par)
{
struct xt_quota_info *q = par->matchinfo;
- BUILD_BUG_ON(sizeof(atomic64_t) != sizeof(__aligned_u64));
+ BUILD_BUG_ON(sizeof(atomic64_t) != sizeof(__u64));
if (q->flags & ~XT_QUOTA_MASK)
return -EINVAL;
--
2.19.0
^ permalink raw reply related
* Re: [PATCH net-next v7 28/28] net: WireGuard secure network tunnel
From: Andrew Lunn @ 2018-10-06 23:54 UTC (permalink / raw)
To: Jason A. Donenfeld; +Cc: linux-kernel, netdev, davem, gregkh
In-Reply-To: <20181006025709.4019-29-Jason@zx2c4.com>
Hi Jason
The BUG() statements all seemed to be removed. Thanks.
We still have 1 errors, 193 warnings, 7529 lines checked from
checkpatch. There are still some Macros flow control statements in
them, despite me pointed them out multiple times.
Im not reviewing this version any further. Until you get the very
basics right, i doubt this patch is going to make much progress
towards inclusion.
Andrew
^ permalink raw reply
* Re: [PATCH net-next 11/19] net: usb: aqc111: Add support for changing MTU
From: Andrew Lunn @ 2018-10-06 16:56 UTC (permalink / raw)
To: Igor Russkikh
Cc: David S . Miller, linux-usb@vger.kernel.org,
netdev@vger.kernel.org, Dmitry Bezrukov
In-Reply-To: <4217a1b53f61c5da8f6ba69326c7b759174817e7.1538734658.git.igor.russkikh@aquantia.com>
> +static int aqc111_change_mtu(struct net_device *net, int new_mtu)
> +{
> + struct usbnet *dev = netdev_priv(net);
> + u16 reg16 = 0;
> + u8 buf[5];
> +
> + if (new_mtu <= 0 || new_mtu > 16334) {
> + netdev_info(net, "Invalid MTU %d requested, hw max 16334",
> + new_mtu);
> + return -EINVAL;
> + }
Please set net->min_mtu, and net->max_mtu, and the core should do this
checking for you. See dev_set_mtu_ext().
Andrew
^ permalink raw reply
* Re: [PATCH net-next 14/19] net: usb: aqc111: Implement set_rx_mode callback
From: Andrew Lunn @ 2018-10-06 17:03 UTC (permalink / raw)
To: Igor Russkikh
Cc: David S . Miller, linux-usb@vger.kernel.org,
netdev@vger.kernel.org, Dmitry Bezrukov
In-Reply-To: <d148df93858d4f8918d4fe5876fcdd308e6dfbef.1538734658.git.igor.russkikh@aquantia.com>
> +static void aqc111_set_rx_mode(struct net_device *net)
> +{
> + struct usbnet *dev = netdev_priv(net);
> + struct aqc111_data *aqc111_data = (struct aqc111_data *)dev->data[0];
> + u8 *m_filter = ((u8 *)dev->data) + 12;
Please could you explain is.
Thanks
Andrew
^ permalink raw reply
* Re: [PATCH net-next 16/19] net: usb: aqc111: Add RX VLAN filtering support
From: Andrew Lunn @ 2018-10-06 17:05 UTC (permalink / raw)
To: Igor Russkikh
Cc: David S . Miller, linux-usb@vger.kernel.org,
netdev@vger.kernel.org, Dmitry Bezrukov
In-Reply-To: <100f2bdc2e23a298afdf240180d224cf39d692ad.1538734658.git.igor.russkikh@aquantia.com>
> @@ -994,6 +1083,7 @@ 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),
Please squash this hunk into the correct patch.
Thanks
Andrew
^ permalink raw reply
* Re: [PATCH net-next 17/19] net: usb: aqc111: Initialize ethtool_ops structure
From: Andrew Lunn @ 2018-10-06 17:08 UTC (permalink / raw)
To: Igor Russkikh
Cc: David S . Miller, linux-usb@vger.kernel.org,
netdev@vger.kernel.org, Dmitry Bezrukov
In-Reply-To: <26309478ca61ae4163acb455d62021cf6801c29a.1538734658.git.igor.russkikh@aquantia.com>
On Fri, Oct 05, 2018 at 10:25:22AM +0000, Igor Russkikh wrote:
> From: Dmitry Bezrukov <dmitry.bezrukov@aquantia.com>
>
> Implement get_drvinfo, set/get_msglevel, get_link callbacks
>
> Signed-off-by: Dmitry Bezrukov <dmitry.bezrukov@aquantia.com>
> Signed-off-by: Igor Russkikh <igor.russkikh@aquantia.com>
> ---
> drivers/net/usb/aqc111.c | 30 ++++++++++++++++++++++++++++++
> 1 file changed, 30 insertions(+)
>
> diff --git a/drivers/net/usb/aqc111.c b/drivers/net/usb/aqc111.c
> index 9908b0a04da6..ade2b60b4811 100644
> --- a/drivers/net/usb/aqc111.c
> +++ b/drivers/net/usb/aqc111.c
> @@ -9,6 +9,7 @@
>
> #include <linux/module.h>
> #include <linux/netdevice.h>
> +#include <linux/ethtool.h>
> #include <linux/mii.h>
> #include <linux/usb.h>
> #include <linux/crc32.h>
> @@ -18,6 +19,9 @@
>
> #include "aqc111.h"
>
> +#define DRIVER_VERSION "1.0.0.0"
Hi Igor
This is been discussed a number of times before, and i expect it to be
discussed again. Driver versions of useless. Please drop it.
> +#define DRIVER_NAME "Aquantia AQtion USB to 5GbE"
It might be nice to include the module name in here, making it easy to
link the module to the driver.
> +
> static int __aqc111_read_cmd(struct usbnet *dev, u8 cmd, u16 value,
> u16 index, u16 size, void *data, int nopm)
> {
> @@ -170,6 +174,24 @@ static int aq_mdio_write_cmd(struct usbnet *dev, u16 value, u16 index,
> return aqc111_write_cmd(dev, AQ_PHY_CMD, value, index, size, data);
> }
>
Andrew
^ permalink raw reply
* Re: [PATCH net-next 06/19] net: usb: aqc111: Introduce link management
From: Andrew Lunn @ 2018-10-06 17:35 UTC (permalink / raw)
To: Igor Russkikh
Cc: David S . Miller, linux-usb@vger.kernel.org,
netdev@vger.kernel.org, Dmitry Bezrukov
In-Reply-To: <cec5cd88988e0985e3fdd1343906ef649b01f646.1538734658.git.igor.russkikh@aquantia.com>
> @@ -202,6 +319,9 @@ static int aqc111_bind(struct usbnet *dev, struct usb_interface *intf)
> dev->net->netdev_ops = &aqc111_netdev_ops;
>
> aqc111_read_fw_version(dev, aqc111_data);
> + aqc111_data->autoneg = AUTONEG_ENABLE;
> + aqc111_data->advertised_speed = (usb_speed == USB_SPEED_SUPER) ?
> + SPEED_5000 : SPEED_1000;
Hi Igor
I'd be interested in knowing the reasoning behind this.
USB 3 has a raw bandwidth of 5Gbps. But it is a shared bus. So you
have no guaranteed you are actually going to get the needed bandwidth
to support line rate.
USB 2.0 only gives you 480Mbps. So it won't even give you the full
1G. So using the same reasoning for USB3, maybe you should limit it to
100Mbps?
I personally would not apply restrictions on the PHY depending on what
USB is being used.
This becomes more important when using SFPs. If i have an SFP peer
which is expecting 2500Base-X, but because the device is plugged into
USB 2 port it is forced to use 1000Base-X, it is not going to get
link.
Andrew
^ permalink raw reply
* Re: [PATCH net-next 18/19] net: usb: aqc111: Implement get/set_link_ksettings callbacks
From: Andrew Lunn @ 2018-10-06 17:38 UTC (permalink / raw)
To: Igor Russkikh
Cc: David S . Miller, linux-usb@vger.kernel.org,
netdev@vger.kernel.org, Dmitry Bezrukov
In-Reply-To: <694b148fa9e79d42fe8a815821ae5b0bc12c4aea.1538734658.git.igor.russkikh@aquantia.com>
> +static int aqc111_set_link_ksettings(struct net_device *net,
> + const 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 = elk->base.speed;
> + u8 autoneg = elk->base.autoneg;
> +
> + if (autoneg == AUTONEG_ENABLE) {
> + if (aqc111_data->autoneg != AUTONEG_ENABLE) {
> + aqc111_data->autoneg = AUTONEG_ENABLE;
> + aqc111_data->advertised_speed =
> + (usb_speed == USB_SPEED_SUPER) ?
> + SPEED_5000 : SPEED_1000;
> + aqc111_set_phy_speed(dev, aqc111_data->autoneg,
> + aqc111_data->advertised_speed);
> + }
> + } else {
> + if (speed != SPEED_100 &&
> + speed != SPEED_1000 &&
> + speed != SPEED_2500 &&
> + speed != SPEED_5000 &&
> + speed != SPEED_UNKNOWN)
> + return -EINVAL;
> +
> + if (usb_speed != USB_SPEED_SUPER && speed > SPEED_1000)
> + return -EINVAL;
> +
Hi Igor
Maybe you are missing a check for duplex here?
> + aqc111_data->autoneg = AUTONEG_DISABLE;
> + if (speed != SPEED_UNKNOWN)
> + aqc111_data->advertised_speed = speed;
> +
> + aqc111_set_phy_speed(dev, aqc111_data->autoneg,
> + aqc111_data->advertised_speed);
> + }
> +
> + return 0;
> +}
> +
^ permalink raw reply
* Re: [PATCH] rtlwifi: rtl8821ae: add in a missing break in switch statement
From: Larry Finger @ 2018-10-07 0:48 UTC (permalink / raw)
To: Joe Perches, Kalle Valo, Colin King
Cc: Ping-Ke Shih, David S . Miller, Tsang-Shian Lin, linux-wireless,
netdev, kernel-janitors, linux-kernel
In-Reply-To: <b0971d2219b7121d06fc2439922a2b759203dd2b.camel@perches.com>
On 10/6/18 5:03 PM, Joe Perches wrote:
> On Sat, 2018-10-06 at 17:00 -0500, Larry Finger wrote:
>> On 10/6/18 3:17 PM, Joe Perches wrote:
>>> On Sat, 2018-10-06 at 15:05 -0500, Larry Finger wrote:
>>>> On 10/6/18 2:30 PM, Kalle Valo wrote:
>>>>> Colin King <colin.king@canonical.com> writes:
>>>>>
>>>>>> From: Colin Ian King <colin.king@canonical.com>
>>>>>>
>>>>>> The switch case RATR_INX_WIRELESS_MC has a missing break, this seems
>>>>>> to be unintentional as the setting of variable ret gets overwritten
>>>>>> when the case falls through to the following RATR_INX_WIRELESS_AC_5N
>>>>>> case. Fix this by adding in the missing break.
>>>>>>
>>>>>> Detected by CoverityScan, CID#1167237 ("Missing break in switch")
>>>>>>
>>>>>> Fixes: 3c05bedb5fef ("Staging: rtl8812ae: Add Realtek 8821 PCI WIFI driver")
>>>>>> Signed-off-by: Colin Ian King <colin.king@canonical.com>
>>>>>> ---
>>>>>> drivers/net/wireless/realtek/rtlwifi/rtl8821ae/hw.c | 1 +
>>>>>
>>>>> Is the fixes line correct? This patch is not for staging.
>>>>
>>>> No, the correct fixes commit is 21e4b0726dc67 (" rtlwifi: rtl8821ae: Move driver
>>>> from staging to regular tree").
>>>>
>>>> This driver was initially placed in staging as it was needed for a special
>>>> project, which is the commit that Colin used. As the patch subject states, the
>>>> driver was later moved to the regular wireless tree.
>>>>
>>>> That break is required, thus ACKed-by: Larry Finger <Larry.Finger@lwfinger.net>
>>>
>>> Why not remove this entirely and use the generic routine in
>>> drivers/net/wireless/realtek/rtlwifi/base.c?
>>>
>>> Is there a real difference?
>>
>> I did not see any difference other than the removal of a bunch of magic numbers
>> and better formatting.
>
> Me neither.
Colin,
Do you want to push the new patch removing the duplicate routine from rtl8821ae?
Larry
^ 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-06 17:49 UTC (permalink / raw)
To: Igor Russkikh
Cc: David S . Miller, linux-usb@vger.kernel.org,
netdev@vger.kernel.org, Dmitry Bezrukov
In-Reply-To: <1a7d4af1b3a6397b76338d543f86bb4c327f5060.1538734658.git.igor.russkikh@aquantia.com>
> + 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.
> +static int aqc111_resume(struct usb_interface *intf)
> +{
> + struct usbnet *dev = usb_get_intfdata(intf);
> + struct aqc111_data *aqc111_data = (struct aqc111_data *)dev->data[0];
> + u8 reg8;
> + u16 reg16;
> +
> + netif_carrier_off(dev->net);
> +
> + /* Power up ethernet PHY */
> + aqc111_data->phy_ops.phy_power = 1;
> + aqc111_data->phy_ops.low_power = 0;
> + aqc111_data->phy_ops.wol = 0;
> + if (aqc111_data->dpa) {
> + aqc111_read_cmd_nopm(dev, AQ_PHY_POWER, 0, 0, 1, ®8);
> + if (reg8 == 0x00) {
> + reg8 = 0x02;
> + aqc111_write_cmd_nopm(dev, AQ_PHY_POWER, 0, 0,
> + 1, ®8);
> + msleep(200);
> + }
> +
> + aq_mdio_read_cmd(dev, AQ_GLB_STD_CTRL_REG, AQ_PHY_GLOBAL_ADDR,
> + 2, ®16);
> + if (reg16 & AQ_PHY_LOW_POWER_MODE) {
> + reg16 &= ~AQ_PHY_LOW_POWER_MODE;
> + aq_mdio_write_cmd(dev, AQ_GLB_STD_CTRL_REG,
> + AQ_PHY_GLOBAL_ADDR, 2, ®16);
> + }
> + }
> +
> + reg8 = 0xFF;
> + aqc111_write_cmd_nopm(dev, AQ_ACCESS_MAC, SFR_BM_INT_MASK,
> + 1, 1, ®8);
> + /* Configure RX control register => start operation */
> + reg16 = aqc111_data->rxctl;
> + reg16 &= ~SFR_RX_CTL_START;
> + aqc111_write_cmd_nopm(dev, AQ_ACCESS_MAC, SFR_RX_CTL, 2, 2, ®16);
> +
> + reg16 |= SFR_RX_CTL_START;
> + aqc111_write_cmd_nopm(dev, AQ_ACCESS_MAC, SFR_RX_CTL, 2, 2, ®16);
> +
> + aqc111_set_phy_speed(dev, aqc111_data->autoneg,
> + aqc111_data->advertised_speed);
> +
Should that be conditional on aqc111_data->dpa?
> +struct aqc111_wol_cfg {
> + u8 hw_addr[6];
> + u8 flags;
> + u8 rsvd[283];
> +};
Do you really need these 283 bytes??
> +
> +#define WOL_CFG_SIZE sizeof(struct aqc111_wol_cfg)
> +
> struct aqc111_data {
> u16 rxctl;
> u8 rx_checksum;
> @@ -228,6 +238,7 @@ struct aqc111_data {
> } fw_ver;
> u8 dpa; /*direct PHY access*/
> struct aqc111_phy_options phy_ops;
> + struct aqc111_wol_cfg wol_cfg;
Those 283 bytes make this whole structure bigger...
Andrew
^ permalink raw reply
* Re: [PATCH net-next 00/19] Add support for Aquantia AQtion USB to 5/2.5GbE devices
From: Andrew Lunn @ 2018-10-06 17:51 UTC (permalink / raw)
To: Igor Russkikh
Cc: David S . Miller, linux-usb@vger.kernel.org,
netdev@vger.kernel.org
In-Reply-To: <cover.1538734658.git.igor.russkikh@aquantia.com>
On Fri, Oct 05, 2018 at 10:24:37AM +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.
>
Hi Igor, Dmitry
Nice patch set, well broken up, easy to review.
Andrew
^ permalink raw reply
* Re: [PATCH net-next] net: dsa: mc88e6xxx: Fix 88E6141/6341 2500mbps SERDES speed
From: Andrew Lunn @ 2018-10-06 18:37 UTC (permalink / raw)
To: Marek Behun; +Cc: netdev, Florian Fainelli, David S . Miller
In-Reply-To: <20181006023810.20be216e@nic.cz>
On Sat, Oct 06, 2018 at 02:38:10AM +0200, Marek Behun wrote:
> > Hi Marek
> >
> > I'm confused.
> >
> > The alt bit is used for configuring 2500. You say 2500 is only
> > supported on port 5. But !port is only true for port 0?
> >
> > Andrew
>
> On Topaz alt_bit is used only for port 0 for differentiating 100 mbps
> vs 200 mbps. The choices for SpdValue are 0 for 10 mbps, 1 for 100
> mbps or 200 mbps (if alt_bit), 2 for 1000 mbps and 3 for 2500 mbps.
> 2500 is allowed only on port 5. alt_bit is not used on Topaz for port
> 5 (serdes), therefore I used !port.
Hi Marek
The commit message could be clearer. It might be going too far, but
you could split this into three.
1) Basic framework code, with works correctly for ports 1-4.
2) The special 200Mbps for port 0
3) The special 2500Mbps for port 5.
But i would also accept a clearer commit message.
Thanks
Andrew
^ permalink raw reply
* Re: [PATCH] rtlwifi: rtl8821ae: add in a missing break in switch statement
From: Larry Finger @ 2018-10-06 20:05 UTC (permalink / raw)
To: Kalle Valo, Colin King
Cc: Ping-Ke Shih, David S . Miller, Tsang-Shian Lin, linux-wireless,
netdev, kernel-janitors, linux-kernel
In-Reply-To: <87r2h2euuy.fsf@kamboji.qca.qualcomm.com>
On 10/6/18 2:30 PM, Kalle Valo wrote:
> Colin King <colin.king@canonical.com> writes:
>
>> From: Colin Ian King <colin.king@canonical.com>
>>
>> The switch case RATR_INX_WIRELESS_MC has a missing break, this seems
>> to be unintentional as the setting of variable ret gets overwritten
>> when the case falls through to the following RATR_INX_WIRELESS_AC_5N
>> case. Fix this by adding in the missing break.
>>
>> Detected by CoverityScan, CID#1167237 ("Missing break in switch")
>>
>> Fixes: 3c05bedb5fef ("Staging: rtl8812ae: Add Realtek 8821 PCI WIFI driver")
>> Signed-off-by: Colin Ian King <colin.king@canonical.com>
>> ---
>> drivers/net/wireless/realtek/rtlwifi/rtl8821ae/hw.c | 1 +
>
> Is the fixes line correct? This patch is not for staging.
No, the correct fixes commit is 21e4b0726dc67 (" rtlwifi: rtl8821ae: Move driver
from staging to regular tree").
This driver was initially placed in staging as it was needed for a special
project, which is the commit that Colin used. As the patch subject states, the
driver was later moved to the regular wireless tree.
That break is required, thus ACKed-by: Larry Finger <Larry.Finger@lwfinger.net>
thanks,
Larry
^ 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