RE: [PATCH net-next 0/2] Add mqprio hardware offload support in hns3 driver

[Yuval Mintz]

> Hi, Yuval
> 
> On 2017/10/15 13:14, Yuval Mintz wrote:
> >> Hi, Yuval
> >>
> >> On 2017/10/13 4:21, Yuval Mintz wrote:
> >>>> This patchset adds a new hardware offload type in mqprio before
> adding
> >>>> mqprio hardware offload support in hns3 driver.
> >>>
> >>> I think one of the biggest issues in tying this to DCB configuration is 
> >>> the
> >>> non-immediate [and possibly non persistent] configuration.
> >>>
> >>> Scenario #1:
> >>> User is configuring mqprio offloaded with 3 TCs while device is in willing
> >> mode.
> >>> Would you expect the driver to immediately respond with a success or
> >> instead
> >>> delay the return until the DCBx negotiation is complete and the
> operational
> >>> num of TCs is actually 3?
> >>
> >> Well, when user requsts the mqprio offloaded by a hardware shared by
> DCB,
> >> I expect
> >> the user is not using the dcb tool.
> >> If user is still using dcb tool, then result is undefined.
> >>
> >> The scenario you mention maybe can be enforced by setting willing to
> zero
> >> when user
> >> is requesting the mqprio offload, and restore the willing bit when
> unloaded
> >> the mqprio
> >> offload.
> >
> > Sounds a bit harsh but would probably work.
> >
> >> But I think the real issue is that dcb and mqprio shares the tc system in 
> >> the
> >> stack,
> >> the problem may be better to be fixed in the stack rather than in the
> driver,
> >> as you
> >> suggested in the DCB patchset. What do you think?
> >
> > What did you have in mind?
> 
> I was thinking maybe the tc system can provide a notification to mqprio and
> dcb.
> mqprio and dcb register a callback to the tc system, when there is some
> change of
> tc configuration, the tc system call the callback from mqprio and dcb.
> 
> >
> >>
> >>>
> >>> Scenario #2:
> >>> Assume user explicitly offloaded mqprio with 3 TCs, but now DCB
> >> configuration
> >>> has changed on the peer side and 4 TCs is the new negotiated
> operational
> >> value.
> >>> Your current driver logic would change the number of TCs underneath
> the
> >> user
> >>> configuration [and it would actually probably work due to mqprio being a
> >> crappy
> >>> qdisc]. But was that the user actual intention?
> >>> [I think the likely answer in this scenario is 'yes' since the 
> >>> alternative is no
> >> better.
> >>> But I still thought it was worth mentioning]
> >>
> >> You are right, the problem also have something to do with mqprio and dcb
> >> sharing
> >> the tc in the stack.
> >>
> >> Druing testing, when user explicitly offloaded mqprio with 3 TCs, all
> >> queue has a default pfifo mqprio attached, after DCB changes the tc num
> to
> >> 4,
> >> using tc qdisc shows some queue does not have a default pfifo mqprio
> >> attached.
> >
> > Really? Then what did it show?
> > [I assume it has some pfifo attached, and it's an mqprio dump kind of an
> issue]
> 
> When queue size of the ndev is 16 and tc num is 3, we set the real queue size
> to
> 15 ( 5 * 3 = 15), mqprio only attach pfifo to the first 15 queue, when tc num
> change
> to 4 by DCB, we set the real queue size to 16 (4 * 4 = 16).
> So tc qdisc shows the last queue has no qdisc attached.

So there is a qdisc attached - mqprio_attach() attches to all transmission
queues [num_tx_queues] and not only the active ones.
But the flow for mqprio might be lacking the additional qdisc_hash_add()
for the additional queue's qdisc.

> 
> >
> >>
> >> Maybe we can add a callback to notify mqprio the configuration has
> changed.
> >>
> >
> > Which would do what?
> > You already have the notifications available for monitoring using dcbnl 
> > logic
> if the
> > configuration change [for user]; So user can re-configure whatever it
> wants.
> 
> Yes, if user is only using dcb tool.
> 
> > But other than dropping all the qdisc configurations and going back to the
> default
> > qdiscs, what default action would mqprio be able to do when configuration
> changes
> > that actually makes sense?
> 
> As explained above, after dcb changing the configuration, some queue may
> have no qdisc
> attached, so I was thinking maybe we can add pfifo to it if there is no qdsic
> attached
> to it.
> 
> Thanks,
> Yunsheng Lin
> 
> >
> >> Thanks
> >> Yunsheng Lin
> >>
> >>>
> >>> Cheers,
> >>> Yuval
> >>>
> >>>>
> >>>> Yunsheng Lin (2):
> >>>>   mqprio: Add a new hardware offload type in mqprio
> >>>>   net: hns3: Add mqprio hardware offload support in hns3 driver
> >>>>
> >>>>  drivers/net/ethernet/hisilicon/hns3/hnae3.h|  1 +
> >>>>  .../net/ethernet/hisilicon/hns3/hns3pf/hclge_dcb.c | 23
> +++
> >>>>  .../net/ethernet/hisilicon/hns3/hns3pf/hns3_enet.c | 46
> >> ++-
> >>>> ---
> >>>>  include/uapi/linux/pkt_sched.h |  1 +
> >>>>  4 files changed, 55 insertions(+), 16 deletions(-)
> >>>>
> >>>> --
> >>>> 1.9.1
> >>>
> >>>
> >>>
> >

RE: [PATCH net-next 0/2] Add mqprio hardware offload support in hns3 driver

[Yuval Mintz]

> Hi, Yuval
> 
> On 2017/10/15 13:14, Yuval Mintz wrote:
> >> Hi, Yuval
> >>
> >> On 2017/10/13 4:21, Yuval Mintz wrote:
> >>>> This patchset adds a new hardware offload type in mqprio before
> adding
> >>>> mqprio hardware offload support in hns3 driver.
> >>>
> >>> I think one of the biggest issues in tying this to DCB configuration is 
> >>> the
> >>> non-immediate [and possibly non persistent] configuration.
> >>>
> >>> Scenario #1:
> >>> User is configuring mqprio offloaded with 3 TCs while device is in willing
> >> mode.
> >>> Would you expect the driver to immediately respond with a success or
> >> instead
> >>> delay the return until the DCBx negotiation is complete and the
> operational
> >>> num of TCs is actually 3?
> >>
> >> Well, when user requsts the mqprio offloaded by a hardware shared by
> DCB,
> >> I expect
> >> the user is not using the dcb tool.
> >> If user is still using dcb tool, then result is undefined.
> >>
> >> The scenario you mention maybe can be enforced by setting willing to
> zero
> >> when user
> >> is requesting the mqprio offload, and restore the willing bit when
> unloaded
> >> the mqprio
> >> offload.
> >
> > Sounds a bit harsh but would probably work.
> >
> >> But I think the real issue is that dcb and mqprio shares the tc system in 
> >> the
> >> stack,
> >> the problem may be better to be fixed in the stack rather than in the
> driver,
> >> as you
> >> suggested in the DCB patchset. What do you think?
> >
> > What did you have in mind?
> 
> I was thinking maybe the tc system can provide a notification to mqprio and
> dcb.
> mqprio and dcb register a callback to the tc system, when there is some
> change of
> tc configuration, the tc system call the callback from mqprio and dcb.
> 
> >
> >>
> >>>
> >>> Scenario #2:
> >>> Assume user explicitly offloaded mqprio with 3 TCs, but now DCB
> >> configuration
> >>> has changed on the peer side and 4 TCs is the new negotiated
> operational
> >> value.
> >>> Your current driver logic would change the number of TCs underneath
> the
> >> user
> >>> configuration [and it would actually probably work due to mqprio being a
> >> crappy
> >>> qdisc]. But was that the user actual intention?
> >>> [I think the likely answer in this scenario is 'yes' since the 
> >>> alternative is no
> >> better.
> >>> But I still thought it was worth mentioning]
> >>
> >> You are right, the problem also have something to do with mqprio and dcb
> >> sharing
> >> the tc in the stack.
> >>
> >> Druing testing, when user explicitly offloaded mqprio with 3 TCs, all
> >> queue has a default pfifo mqprio attached, after DCB changes the tc num
> to
> >> 4,
> >> using tc qdisc shows some queue does not have a default pfifo mqprio
> >> attached.
> >
> > Really? Then what did it show?
> > [I assume it has some pfifo attached, and it's an mqprio dump kind of an
> issue]
> 
> When queue size of the ndev is 16 and tc num is 3, we set the real queue size
> to
> 15 ( 5 * 3 = 15), mqprio only attach pfifo to the first 15 queue, when tc num
> change
> to 4 by DCB, we set the real queue size to 16 (4 * 4 = 16).
> So tc qdisc shows the last queue has no qdisc attached.

So there is a qdisc attached - mqprio_attach() attches to all transmission
queues [num_tx_queues] and not only the active ones.
But the flow for mqprio might be lacking the additional qdisc_hash_add()
for the additional queue's qdisc.

> 
> >
> >>
> >> Maybe we can add a callback to notify mqprio the configuration has
> changed.
> >>
> >
> > Which would do what?
> > You already have the notifications available for monitoring using dcbnl 
> > logic
> if the
> > configuration change [for user]; So user can re-configure whatever it
> wants.
> 
> Yes, if user is only using dcb tool.
> 
> > But other than dropping all the qdisc configurations and going back to the
> default
> > qdiscs, what default action would mqprio be able to do when configuration
> changes
> > that actually makes sense?
> 
> As explained above, after dcb changing the configuration, some queue may
> have no qdisc
> attached, so I was thinking maybe we can add pfifo to it if there is no qdsic
> attached
> to it.
> 
> Thanks,
> Yunsheng Lin
> 
> >
> >> Thanks
> >> Yunsheng Lin
> >>
> >>>
> >>> Cheers,
> >>> Yuval
> >>>
> >>>>
> >>>> Yunsheng Lin (2):
> >>>>   mqprio: Add a new hardware offload type in mqprio
> >>>>   net: hns3: Add mqprio hardware offload support in hns3 driver
> >>>>
> >>>>  drivers/net/ethernet/hisilicon/hns3/hnae3.h|  1 +
> >>>>  .../net/ethernet/hisilicon/hns3/hns3pf/hclge_dcb.c | 23
> +++
> >>>>  .../net/ethernet/hisilicon/hns3/hns3pf/hns3_enet.c | 46
> >> ++-
> >>>> ---
> >>>>  include/uapi/linux/pkt_sched.h |  1 +
> >>>>  4 files changed, 55 insertions(+), 16 deletions(-)
> >>>>
> >>>> --
> >>>> 1.9.1
> >>>
> >>>
> >>>
> >

RE: [PATCH net-next 0/2] Add mqprio hardware offload support in hns3 driver

[Yuval Mintz]

> > >> This patchset adds a new hardware offload type in mqprio before
> adding
> > >> mqprio hardware offload support in hns3 driver.

Apparently Dave has already acceptedAmirtha's changes to mqprio:
https://marc.info/?l=linux-netdev=150803219824053=2 
so I guess you need to revise your patchs to align to the new conventions.

> > >
> > > I think one of the biggest issues in tying this to DCB configuration is 
> > > the
> > > non-immediate [and possibly non persistent] configuration.
> > >
> > > Scenario #1:
> > > User is configuring mqprio offloaded with 3 TCs while device is in willing
> > mode.
> > > Would you expect the driver to immediately respond with a success or
> > instead
> > > delay the return until the DCBx negotiation is complete and the
> operational
> > > num of TCs is actually 3?
> >
> > Well, when user requsts the mqprio offloaded by a hardware shared by
> DCB,
> > I expect
> > the user is not using the dcb tool.
> > If user is still using dcb tool, then result is undefined.
> >
> > The scenario you mention maybe can be enforced by setting willing to zero
> > when user
> > is requesting the mqprio offload, and restore the willing bit when unloaded
> > the mqprio
> > offload.
> 
> Sounds a bit harsh but would probably work.
> 
> > But I think the real issue is that dcb and mqprio shares the tc system in 
> > the
> > stack,
> > the problem may be better to be fixed in the stack rather than in the
> driver,
> > as you
> > suggested in the DCB patchset. What do you think?
> 
> What did you have in mind?
> 
> >
> > >
> > > Scenario #2:
> > > Assume user explicitly offloaded mqprio with 3 TCs, but now DCB
> > configuration
> > > has changed on the peer side and 4 TCs is the new negotiated operational
> > value.
> > > Your current driver logic would change the number of TCs underneath
> the
> > user
> > > configuration [and it would actually probably work due to mqprio being a
> > crappy
> > > qdisc]. But was that the user actual intention?
> > > [I think the likely answer in this scenario is 'yes' since the 
> > > alternative is no
> > better.
> > > But I still thought it was worth mentioning]
> >
> > You are right, the problem also have something to do with mqprio and dcb
> > sharing
> > the tc in the stack.
> >
> > Druing testing, when user explicitly offloaded mqprio with 3 TCs, all
> > queue has a default pfifo mqprio attached, after DCB changes the tc num
> to
> > 4,
> > using tc qdisc shows some queue does not have a default pfifo mqprio
> > attached.
> 
> Really? Then what did it show?
> [I assume it has some pfifo attached, and it's an mqprio dump kind of an
> issue]
> 
> >
> > Maybe we can add a callback to notify mqprio the configuration has
> changed.
> >
> 
> Which would do what?
> You already have the notifications available for monitoring using dcbnl logic 
> if
> the
> configuration change [for user]; So user can re-configure whatever it wants.
> But other than dropping all the qdisc configurations and going back to the
> default
> qdiscs, what default action would mqprio be able to do when configuration
> changes
> that actually makes sense?
> 
> > Thanks
> > Yunsheng Lin
> >
> > >
> > > Cheers,
> > > Yuval
> > >
> > >>
> > >> Yunsheng Lin (2):
> > >>   mqprio: Add a new hardware offload type in mqprio
> > >>   net: hns3: Add mqprio hardware offload support in hns3 driver
> > >>
> > >>  drivers/net/ethernet/hisilicon/hns3/hnae3.h|  1 +
> > >>  .../net/ethernet/hisilicon/hns3/hns3pf/hclge_dcb.c | 23 +++
> > >>  .../net/ethernet/hisilicon/hns3/hns3pf/hns3_enet.c | 46
> > ++-
> > >> ---
> > >>  include/uapi/linux/pkt_sched.h |  1 +
> > >>  4 files changed, 55 insertions(+), 16 deletions(-)
> > >>
> > >> --
> > >> 1.9.1
> > >
> > >
> > >

RE: [PATCH net-next 0/2] Add mqprio hardware offload support in hns3 driver

[Yuval Mintz]

> > >> This patchset adds a new hardware offload type in mqprio before
> adding
> > >> mqprio hardware offload support in hns3 driver.

Apparently Dave has already acceptedAmirtha's changes to mqprio:
https://marc.info/?l=linux-netdev=150803219824053=2 
so I guess you need to revise your patchs to align to the new conventions.

> > >
> > > I think one of the biggest issues in tying this to DCB configuration is 
> > > the
> > > non-immediate [and possibly non persistent] configuration.
> > >
> > > Scenario #1:
> > > User is configuring mqprio offloaded with 3 TCs while device is in willing
> > mode.
> > > Would you expect the driver to immediately respond with a success or
> > instead
> > > delay the return until the DCBx negotiation is complete and the
> operational
> > > num of TCs is actually 3?
> >
> > Well, when user requsts the mqprio offloaded by a hardware shared by
> DCB,
> > I expect
> > the user is not using the dcb tool.
> > If user is still using dcb tool, then result is undefined.
> >
> > The scenario you mention maybe can be enforced by setting willing to zero
> > when user
> > is requesting the mqprio offload, and restore the willing bit when unloaded
> > the mqprio
> > offload.
> 
> Sounds a bit harsh but would probably work.
> 
> > But I think the real issue is that dcb and mqprio shares the tc system in 
> > the
> > stack,
> > the problem may be better to be fixed in the stack rather than in the
> driver,
> > as you
> > suggested in the DCB patchset. What do you think?
> 
> What did you have in mind?
> 
> >
> > >
> > > Scenario #2:
> > > Assume user explicitly offloaded mqprio with 3 TCs, but now DCB
> > configuration
> > > has changed on the peer side and 4 TCs is the new negotiated operational
> > value.
> > > Your current driver logic would change the number of TCs underneath
> the
> > user
> > > configuration [and it would actually probably work due to mqprio being a
> > crappy
> > > qdisc]. But was that the user actual intention?
> > > [I think the likely answer in this scenario is 'yes' since the 
> > > alternative is no
> > better.
> > > But I still thought it was worth mentioning]
> >
> > You are right, the problem also have something to do with mqprio and dcb
> > sharing
> > the tc in the stack.
> >
> > Druing testing, when user explicitly offloaded mqprio with 3 TCs, all
> > queue has a default pfifo mqprio attached, after DCB changes the tc num
> to
> > 4,
> > using tc qdisc shows some queue does not have a default pfifo mqprio
> > attached.
> 
> Really? Then what did it show?
> [I assume it has some pfifo attached, and it's an mqprio dump kind of an
> issue]
> 
> >
> > Maybe we can add a callback to notify mqprio the configuration has
> changed.
> >
> 
> Which would do what?
> You already have the notifications available for monitoring using dcbnl logic 
> if
> the
> configuration change [for user]; So user can re-configure whatever it wants.
> But other than dropping all the qdisc configurations and going back to the
> default
> qdiscs, what default action would mqprio be able to do when configuration
> changes
> that actually makes sense?
> 
> > Thanks
> > Yunsheng Lin
> >
> > >
> > > Cheers,
> > > Yuval
> > >
> > >>
> > >> Yunsheng Lin (2):
> > >>   mqprio: Add a new hardware offload type in mqprio
> > >>   net: hns3: Add mqprio hardware offload support in hns3 driver
> > >>
> > >>  drivers/net/ethernet/hisilicon/hns3/hnae3.h|  1 +
> > >>  .../net/ethernet/hisilicon/hns3/hns3pf/hclge_dcb.c | 23 +++
> > >>  .../net/ethernet/hisilicon/hns3/hns3pf/hns3_enet.c | 46
> > ++-
> > >> ---
> > >>  include/uapi/linux/pkt_sched.h |  1 +
> > >>  4 files changed, 55 insertions(+), 16 deletions(-)
> > >>
> > >> --
> > >> 1.9.1
> > >
> > >
> > >

RE: [PATCH net-next 0/2] Add mqprio hardware offload support in hns3 driver

[Yuval Mintz]

> Hi, Yuval
> 
> On 2017/10/13 4:21, Yuval Mintz wrote:
> >> This patchset adds a new hardware offload type in mqprio before adding
> >> mqprio hardware offload support in hns3 driver.
> >
> > I think one of the biggest issues in tying this to DCB configuration is the
> > non-immediate [and possibly non persistent] configuration.
> >
> > Scenario #1:
> > User is configuring mqprio offloaded with 3 TCs while device is in willing
> mode.
> > Would you expect the driver to immediately respond with a success or
> instead
> > delay the return until the DCBx negotiation is complete and the operational
> > num of TCs is actually 3?
> 
> Well, when user requsts the mqprio offloaded by a hardware shared by DCB,
> I expect
> the user is not using the dcb tool.
> If user is still using dcb tool, then result is undefined.
> 
> The scenario you mention maybe can be enforced by setting willing to zero
> when user
> is requesting the mqprio offload, and restore the willing bit when unloaded
> the mqprio
> offload.

Sounds a bit harsh but would probably work.

> But I think the real issue is that dcb and mqprio shares the tc system in the
> stack,
> the problem may be better to be fixed in the stack rather than in the driver,
> as you
> suggested in the DCB patchset. What do you think?

What did you have in mind?

> 
> >
> > Scenario #2:
> > Assume user explicitly offloaded mqprio with 3 TCs, but now DCB
> configuration
> > has changed on the peer side and 4 TCs is the new negotiated operational
> value.
> > Your current driver logic would change the number of TCs underneath the
> user
> > configuration [and it would actually probably work due to mqprio being a
> crappy
> > qdisc]. But was that the user actual intention?
> > [I think the likely answer in this scenario is 'yes' since the alternative 
> > is no
> better.
> > But I still thought it was worth mentioning]
> 
> You are right, the problem also have something to do with mqprio and dcb
> sharing
> the tc in the stack.
> 
> Druing testing, when user explicitly offloaded mqprio with 3 TCs, all
> queue has a default pfifo mqprio attached, after DCB changes the tc num to
> 4,
> using tc qdisc shows some queue does not have a default pfifo mqprio
> attached.

Really? Then what did it show? 
[I assume it has some pfifo attached, and it's an mqprio dump kind of an issue]

> 
> Maybe we can add a callback to notify mqprio the configuration has changed.
> 

Which would do what?
You already have the notifications available for monitoring using dcbnl logic 
if the
configuration change [for user]; So user can re-configure whatever it wants.
But other than dropping all the qdisc configurations and going back to the 
default
qdiscs, what default action would mqprio be able to do when configuration 
changes
that actually makes sense?

> Thanks
> Yunsheng Lin
> 
> >
> > Cheers,
> > Yuval
> >
> >>
> >> Yunsheng Lin (2):
> >>   mqprio: Add a new hardware offload type in mqprio
> >>   net: hns3: Add mqprio hardware offload support in hns3 driver
> >>
> >>  drivers/net/ethernet/hisilicon/hns3/hnae3.h|  1 +
> >>  .../net/ethernet/hisilicon/hns3/hns3pf/hclge_dcb.c | 23 +++
> >>  .../net/ethernet/hisilicon/hns3/hns3pf/hns3_enet.c | 46
> ++-
> >> ---
> >>  include/uapi/linux/pkt_sched.h |  1 +
> >>  4 files changed, 55 insertions(+), 16 deletions(-)
> >>
> >> --
> >> 1.9.1
> >
> >
> >

RE: [PATCH net-next 0/2] Add mqprio hardware offload support in hns3 driver

[Yuval Mintz]

> Hi, Yuval
> 
> On 2017/10/13 4:21, Yuval Mintz wrote:
> >> This patchset adds a new hardware offload type in mqprio before adding
> >> mqprio hardware offload support in hns3 driver.
> >
> > I think one of the biggest issues in tying this to DCB configuration is the
> > non-immediate [and possibly non persistent] configuration.
> >
> > Scenario #1:
> > User is configuring mqprio offloaded with 3 TCs while device is in willing
> mode.
> > Would you expect the driver to immediately respond with a success or
> instead
> > delay the return until the DCBx negotiation is complete and the operational
> > num of TCs is actually 3?
> 
> Well, when user requsts the mqprio offloaded by a hardware shared by DCB,
> I expect
> the user is not using the dcb tool.
> If user is still using dcb tool, then result is undefined.
> 
> The scenario you mention maybe can be enforced by setting willing to zero
> when user
> is requesting the mqprio offload, and restore the willing bit when unloaded
> the mqprio
> offload.

Sounds a bit harsh but would probably work.

> But I think the real issue is that dcb and mqprio shares the tc system in the
> stack,
> the problem may be better to be fixed in the stack rather than in the driver,
> as you
> suggested in the DCB patchset. What do you think?

What did you have in mind?

> 
> >
> > Scenario #2:
> > Assume user explicitly offloaded mqprio with 3 TCs, but now DCB
> configuration
> > has changed on the peer side and 4 TCs is the new negotiated operational
> value.
> > Your current driver logic would change the number of TCs underneath the
> user
> > configuration [and it would actually probably work due to mqprio being a
> crappy
> > qdisc]. But was that the user actual intention?
> > [I think the likely answer in this scenario is 'yes' since the alternative 
> > is no
> better.
> > But I still thought it was worth mentioning]
> 
> You are right, the problem also have something to do with mqprio and dcb
> sharing
> the tc in the stack.
> 
> Druing testing, when user explicitly offloaded mqprio with 3 TCs, all
> queue has a default pfifo mqprio attached, after DCB changes the tc num to
> 4,
> using tc qdisc shows some queue does not have a default pfifo mqprio
> attached.

Really? Then what did it show? 
[I assume it has some pfifo attached, and it's an mqprio dump kind of an issue]

> 
> Maybe we can add a callback to notify mqprio the configuration has changed.
> 

Which would do what?
You already have the notifications available for monitoring using dcbnl logic 
if the
configuration change [for user]; So user can re-configure whatever it wants.
But other than dropping all the qdisc configurations and going back to the 
default
qdiscs, what default action would mqprio be able to do when configuration 
changes
that actually makes sense?

> Thanks
> Yunsheng Lin
> 
> >
> > Cheers,
> > Yuval
> >
> >>
> >> Yunsheng Lin (2):
> >>   mqprio: Add a new hardware offload type in mqprio
> >>   net: hns3: Add mqprio hardware offload support in hns3 driver
> >>
> >>  drivers/net/ethernet/hisilicon/hns3/hnae3.h|  1 +
> >>  .../net/ethernet/hisilicon/hns3/hns3pf/hclge_dcb.c | 23 +++
> >>  .../net/ethernet/hisilicon/hns3/hns3pf/hns3_enet.c | 46
> ++-
> >> ---
> >>  include/uapi/linux/pkt_sched.h |  1 +
> >>  4 files changed, 55 insertions(+), 16 deletions(-)
> >>
> >> --
> >> 1.9.1
> >
> >
> >

RE: [PATCH net-next 0/2] Add mqprio hardware offload support in hns3 driver

[Yuval Mintz]

> This patchset adds a new hardware offload type in mqprio before adding
> mqprio hardware offload support in hns3 driver.

I think one of the biggest issues in tying this to DCB configuration is the
non-immediate [and possibly non persistent] configuration.

Scenario #1:
User is configuring mqprio offloaded with 3 TCs while device is in willing mode.
Would you expect the driver to immediately respond with a success or instead
delay the return until the DCBx negotiation is complete and the operational
num of TCs is actually 3?

Scenario #2:
Assume user explicitly offloaded mqprio with 3 TCs, but now DCB configuration
has changed on the peer side and 4 TCs is the new negotiated operational value.
Your current driver logic would change the number of TCs underneath the user
configuration [and it would actually probably work due to mqprio being a crappy
qdisc]. But was that the user actual intention?
[I think the likely answer in this scenario is 'yes' since the alternative is 
no better.
But I still thought it was worth mentioning]

Cheers,
Yuval

> 
> Yunsheng Lin (2):
>   mqprio: Add a new hardware offload type in mqprio
>   net: hns3: Add mqprio hardware offload support in hns3 driver
> 
>  drivers/net/ethernet/hisilicon/hns3/hnae3.h|  1 +
>  .../net/ethernet/hisilicon/hns3/hns3pf/hclge_dcb.c | 23 +++
>  .../net/ethernet/hisilicon/hns3/hns3pf/hns3_enet.c | 46 ++-
> ---
>  include/uapi/linux/pkt_sched.h |  1 +
>  4 files changed, 55 insertions(+), 16 deletions(-)
> 
> --
> 1.9.1

RE: [PATCH net-next 0/2] Add mqprio hardware offload support in hns3 driver

[Yuval Mintz]

> This patchset adds a new hardware offload type in mqprio before adding
> mqprio hardware offload support in hns3 driver.

I think one of the biggest issues in tying this to DCB configuration is the
non-immediate [and possibly non persistent] configuration.

Scenario #1:
User is configuring mqprio offloaded with 3 TCs while device is in willing mode.
Would you expect the driver to immediately respond with a success or instead
delay the return until the DCBx negotiation is complete and the operational
num of TCs is actually 3?

Scenario #2:
Assume user explicitly offloaded mqprio with 3 TCs, but now DCB configuration
has changed on the peer side and 4 TCs is the new negotiated operational value.
Your current driver logic would change the number of TCs underneath the user
configuration [and it would actually probably work due to mqprio being a crappy
qdisc]. But was that the user actual intention?
[I think the likely answer in this scenario is 'yes' since the alternative is 
no better.
But I still thought it was worth mentioning]

Cheers,
Yuval

> 
> Yunsheng Lin (2):
>   mqprio: Add a new hardware offload type in mqprio
>   net: hns3: Add mqprio hardware offload support in hns3 driver
> 
>  drivers/net/ethernet/hisilicon/hns3/hnae3.h|  1 +
>  .../net/ethernet/hisilicon/hns3/hns3pf/hclge_dcb.c | 23 +++
>  .../net/ethernet/hisilicon/hns3/hns3pf/hns3_enet.c | 46 ++-
> ---
>  include/uapi/linux/pkt_sched.h |  1 +
>  4 files changed, 55 insertions(+), 16 deletions(-)
> 
> --
> 1.9.1

RE: [PATCH net-next 1/2] mqprio: Add a new hardware offload type in mqprio

[Yuval Mintz]

> When a driver supports both dcb and hardware offloaded mqprio, and
> user is running mqprio and dcb tool concurrently, the configuration
> set by each tool may be conflicted with each other because the dcb
(for second 'each') s/each/the

> and mqprio may be using the same hardwere offload component and share
s/hardwere/hardware

> the tc system in the network stack.
> 
> This patch adds a new offload type to indicate that the underlying
> driver offload prio mapping as part of DCB. If the driver would be
'should' offload

> incapable of that it would refuse the offload. User would then have
> to explicitly request that qdisc offload.

RE: [PATCH net-next 1/2] mqprio: Add a new hardware offload type in mqprio

[Yuval Mintz]

> When a driver supports both dcb and hardware offloaded mqprio, and
> user is running mqprio and dcb tool concurrently, the configuration
> set by each tool may be conflicted with each other because the dcb
(for second 'each') s/each/the

> and mqprio may be using the same hardwere offload component and share
s/hardwere/hardware

> the tc system in the network stack.
> 
> This patch adds a new offload type to indicate that the underlying
> driver offload prio mapping as part of DCB. If the driver would be
'should' offload

> incapable of that it would refuse the offload. User would then have
> to explicitly request that qdisc offload.

RE: [PATCH v2 net-next 10/10] net: hns3: Add mqprio support when interacting with network stack

[Yuval Mintz]

> Hi, Yuval
> 
> On 2017/9/26 14:43, Yuval Mintz wrote:
> >> When using tc qdisc to configure DCB parameter, dcb_ops->setup_tc
> >> is used to tell hclge_dcb module to do the setup.
> >
> > While this might be a step in the right direction, this causes an 
> > inconsistency
> > in user experience - Some [well, most] vendors didn't allow the mqprio
> > priority mapping to affect DCB, instead relying on the dcbnl functionality
> > to control that configuration.
> >
> > A couple of options to consider:
> >   - Perhaps said logic shouldn't be contained inside the driver but rather
> >  in mqprio logic itself. I.e., rely on DCBNL functionality [if 
> > available] from
> >  within mqprio and try changing the configuration.
> 
> In net/dcb/dcbnl.c
> dcbnl_ieee_set already call dcbnl_ieee_notify to notify the user space
> configuration has changed, does this dcbnl_ieee_notify function do the
> job for us? I am not sure if lldpad has registered for this notifition.

Not that familiar with the dcbnl calls; Shouldn't dcbnl_setall be called to
make the configuration apply [or is that only for ieee]?
Regardless, don't know if it makes sense to assume user-application would
fix the qdisc configuration by notification while dcbnl logic in kernel could 
have
done that instead.

> As you suggested below, can we add a new TC_MQPRIO_HW_OFFLOAD_
> value to
> reflect that the configuration is needed to be changed by dcbnl_ieee_set
> (perhaps some other function) in dcbnl?
> Do you think it is feasible?

Either I'm miseading your answer or we think of it from 2 opposite end.
I was thinking that the new offloaded flag would indicate to the underlying
driver that it's expected to offload the prio mapping [as part of DCB].
If the driver would be incapable of that it would refuse the offload.
User would then have to explicitly request that the qdisc offload.

> 
> 
> >   - Add a new TC_MQPRIO_HW_OFFLOAD_ value to explicitly reflect user
> >  request to allow this configuration to affect DCB.
> >
> >> When using lldptool to configure DCB parameter, hclge_dcb module
> >> call the client_ops->setup_tc to tell network stack which queue
> >> and priority is using for specific tc.
> >
> > You're basically bypassing the mqprio logic.
> > Since you're configuring the prio->queue mapping from DCB flow,
> > you'll get an mqprio-like behavior [meaning a transmitted packet
> > would reach a transmission queue associated with its priority] even
> > if device wasn't grated with an mqprio qdisc.
> > Why should your user even use mqprio? What benefit does he get from it?
> >
> > ...
> >
> >> +static int hns3_nic_set_real_num_queue(struct net_device *netdev)
> >> +{
> >> +  struct hns3_nic_priv *priv = netdev_priv(netdev);
> >> +  struct hnae3_handle *h = priv->ae_handle;
> >> +  struct hnae3_knic_private_info *kinfo = >kinfo;
> >> +  unsigned int queue_size = kinfo->rss_size * kinfo->num_tc;
> >> +  int ret;
> >> +
> >> +  ret = netif_set_real_num_tx_queues(netdev, queue_size);
> >> +  if (ret) {
> >> +  netdev_err(netdev,
> >> + "netif_set_real_num_tx_queues fail, ret=%d!\n",
> >> + ret);
> >> +  return ret;
> >> +  }
> >> +
> >> +  ret = netif_set_real_num_rx_queues(netdev, queue_size);
> >
> > I don't think you're changing the driver behavior, but why are you setting
> > the real number of rx queues based on the number of TCs?
> > Do you actually open (TC x RSS) Rx queues?
> >
> > .
> >

RE: [PATCH v2 net-next 10/10] net: hns3: Add mqprio support when interacting with network stack

[Yuval Mintz]

> Hi, Yuval
> 
> On 2017/9/26 14:43, Yuval Mintz wrote:
> >> When using tc qdisc to configure DCB parameter, dcb_ops->setup_tc
> >> is used to tell hclge_dcb module to do the setup.
> >
> > While this might be a step in the right direction, this causes an 
> > inconsistency
> > in user experience - Some [well, most] vendors didn't allow the mqprio
> > priority mapping to affect DCB, instead relying on the dcbnl functionality
> > to control that configuration.
> >
> > A couple of options to consider:
> >   - Perhaps said logic shouldn't be contained inside the driver but rather
> >  in mqprio logic itself. I.e., rely on DCBNL functionality [if 
> > available] from
> >  within mqprio and try changing the configuration.
> 
> In net/dcb/dcbnl.c
> dcbnl_ieee_set already call dcbnl_ieee_notify to notify the user space
> configuration has changed, does this dcbnl_ieee_notify function do the
> job for us? I am not sure if lldpad has registered for this notifition.

Not that familiar with the dcbnl calls; Shouldn't dcbnl_setall be called to
make the configuration apply [or is that only for ieee]?
Regardless, don't know if it makes sense to assume user-application would
fix the qdisc configuration by notification while dcbnl logic in kernel could 
have
done that instead.

> As you suggested below, can we add a new TC_MQPRIO_HW_OFFLOAD_
> value to
> reflect that the configuration is needed to be changed by dcbnl_ieee_set
> (perhaps some other function) in dcbnl?
> Do you think it is feasible?

Either I'm miseading your answer or we think of it from 2 opposite end.
I was thinking that the new offloaded flag would indicate to the underlying
driver that it's expected to offload the prio mapping [as part of DCB].
If the driver would be incapable of that it would refuse the offload.
User would then have to explicitly request that the qdisc offload.

> 
> 
> >   - Add a new TC_MQPRIO_HW_OFFLOAD_ value to explicitly reflect user
> >  request to allow this configuration to affect DCB.
> >
> >> When using lldptool to configure DCB parameter, hclge_dcb module
> >> call the client_ops->setup_tc to tell network stack which queue
> >> and priority is using for specific tc.
> >
> > You're basically bypassing the mqprio logic.
> > Since you're configuring the prio->queue mapping from DCB flow,
> > you'll get an mqprio-like behavior [meaning a transmitted packet
> > would reach a transmission queue associated with its priority] even
> > if device wasn't grated with an mqprio qdisc.
> > Why should your user even use mqprio? What benefit does he get from it?
> >
> > ...
> >
> >> +static int hns3_nic_set_real_num_queue(struct net_device *netdev)
> >> +{
> >> +  struct hns3_nic_priv *priv = netdev_priv(netdev);
> >> +  struct hnae3_handle *h = priv->ae_handle;
> >> +  struct hnae3_knic_private_info *kinfo = >kinfo;
> >> +  unsigned int queue_size = kinfo->rss_size * kinfo->num_tc;
> >> +  int ret;
> >> +
> >> +  ret = netif_set_real_num_tx_queues(netdev, queue_size);
> >> +  if (ret) {
> >> +  netdev_err(netdev,
> >> + "netif_set_real_num_tx_queues fail, ret=%d!\n",
> >> + ret);
> >> +  return ret;
> >> +  }
> >> +
> >> +  ret = netif_set_real_num_rx_queues(netdev, queue_size);
> >
> > I don't think you're changing the driver behavior, but why are you setting
> > the real number of rx queues based on the number of TCs?
> > Do you actually open (TC x RSS) Rx queues?
> >
> > .
> >

RE: [PATCH v2 net-next 10/10] net: hns3: Add mqprio support when interacting with network stack

[Yuval Mintz]

> When using tc qdisc to configure DCB parameter, dcb_ops->setup_tc
> is used to tell hclge_dcb module to do the setup.

While this might be a step in the right direction, this causes an inconsistency
in user experience - Some [well, most] vendors didn't allow the mqprio
priority mapping to affect DCB, instead relying on the dcbnl functionality
to control that configuration.

A couple of options to consider:
  - Perhaps said logic shouldn't be contained inside the driver but rather
 in mqprio logic itself. I.e., rely on DCBNL functionality [if available] 
from
 within mqprio and try changing the configuration. 
  - Add a new TC_MQPRIO_HW_OFFLOAD_ value to explicitly reflect user
 request to allow this configuration to affect DCB.

> When using lldptool to configure DCB parameter, hclge_dcb module
> call the client_ops->setup_tc to tell network stack which queue
> and priority is using for specific tc.

You're basically bypassing the mqprio logic.
Since you're configuring the prio->queue mapping from DCB flow,
you'll get an mqprio-like behavior [meaning a transmitted packet
would reach a transmission queue associated with its priority] even
if device wasn't grated with an mqprio qdisc.
Why should your user even use mqprio? What benefit does he get from it?

...

> +static int hns3_nic_set_real_num_queue(struct net_device *netdev)
> +{
> + struct hns3_nic_priv *priv = netdev_priv(netdev);
> + struct hnae3_handle *h = priv->ae_handle;
> + struct hnae3_knic_private_info *kinfo = >kinfo;
> + unsigned int queue_size = kinfo->rss_size * kinfo->num_tc;
> + int ret;
> +
> + ret = netif_set_real_num_tx_queues(netdev, queue_size);
> + if (ret) {
> + netdev_err(netdev,
> +"netif_set_real_num_tx_queues fail, ret=%d!\n",
> +ret);
> + return ret;
> + }
> +
> + ret = netif_set_real_num_rx_queues(netdev, queue_size);

I don't think you're changing the driver behavior, but why are you setting
the real number of rx queues based on the number of TCs?
Do you actually open (TC x RSS) Rx queues?

RE: [PATCH v2 net-next 10/10] net: hns3: Add mqprio support when interacting with network stack

[Yuval Mintz]

> When using tc qdisc to configure DCB parameter, dcb_ops->setup_tc
> is used to tell hclge_dcb module to do the setup.

While this might be a step in the right direction, this causes an inconsistency
in user experience - Some [well, most] vendors didn't allow the mqprio
priority mapping to affect DCB, instead relying on the dcbnl functionality
to control that configuration.

A couple of options to consider:
  - Perhaps said logic shouldn't be contained inside the driver but rather
 in mqprio logic itself. I.e., rely on DCBNL functionality [if available] 
from
 within mqprio and try changing the configuration. 
  - Add a new TC_MQPRIO_HW_OFFLOAD_ value to explicitly reflect user
 request to allow this configuration to affect DCB.

> When using lldptool to configure DCB parameter, hclge_dcb module
> call the client_ops->setup_tc to tell network stack which queue
> and priority is using for specific tc.

You're basically bypassing the mqprio logic.
Since you're configuring the prio->queue mapping from DCB flow,
you'll get an mqprio-like behavior [meaning a transmitted packet
would reach a transmission queue associated with its priority] even
if device wasn't grated with an mqprio qdisc.
Why should your user even use mqprio? What benefit does he get from it?

...

> +static int hns3_nic_set_real_num_queue(struct net_device *netdev)
> +{
> + struct hns3_nic_priv *priv = netdev_priv(netdev);
> + struct hnae3_handle *h = priv->ae_handle;
> + struct hnae3_knic_private_info *kinfo = >kinfo;
> + unsigned int queue_size = kinfo->rss_size * kinfo->num_tc;
> + int ret;
> +
> + ret = netif_set_real_num_tx_queues(netdev, queue_size);
> + if (ret) {
> + netdev_err(netdev,
> +"netif_set_real_num_tx_queues fail, ret=%d!\n",
> +ret);
> + return ret;
> + }
> +
> + ret = netif_set_real_num_rx_queues(netdev, queue_size);

I don't think you're changing the driver behavior, but why are you setting
the real number of rx queues based on the number of TCs?
Do you actually open (TC x RSS) Rx queues?

RE: [PATCH v2] qed: mark symbols static where possible

[Yuval Mintz]

> 
> 
> In fact, these functions are only used in the file in which they are
> declared and don't need a declaration, but can be made static.
> so this patch marks these functions with 'static'.
...
> -void qed_get_vport_stats(struct qed_dev *cdev,
> -  struct qed_eth_stats *stats)
> +static void qed_get_vport_stats(struct qed_dev *cdev,
>+  struct qed_eth_stats *stats)

Looks like you've missed my comment from v1 -
> 1. qed_get_vport_stats() is in use in net-next by qed_main.c starting 
> with
> 6c75424612a7 ("qed: Add support for NCSI statistics."), so we 
> shouldn't make it static.

Besides, it still doesn't apply to net-next
[as net-next has function declaration as a single liner].

RE: [PATCH v2] qed: mark symbols static where possible

[Yuval Mintz]

> 
> 
> In fact, these functions are only used in the file in which they are
> declared and don't need a declaration, but can be made static.
> so this patch marks these functions with 'static'.
...
> -void qed_get_vport_stats(struct qed_dev *cdev,
> -  struct qed_eth_stats *stats)
> +static void qed_get_vport_stats(struct qed_dev *cdev,
>+  struct qed_eth_stats *stats)

Looks like you've missed my comment from v1 -
> 1. qed_get_vport_stats() is in use in net-next by qed_main.c starting 
> with
> 6c75424612a7 ("qed: Add support for NCSI statistics."), so we 
> shouldn't make it static.

Besides, it still doesn't apply to net-next
[as net-next has function declaration as a single liner].

RE: [PATCH] qede: mark qede_set_features() static

[Yuval Mintz]

> We get 1 warning when building kernel with W=1:
> drivers/net/ethernet/qlogic/qede/qede_main.c:2113:5: warning: no previous
> prototype for 'qede_set_features' [-Wmissing-prototypes]
> 
> In fact, this function is only used in the file in which it is declared and 
> don't need
> a declaration, but can be made static.
> so this patch marks this function with 'static'.
> 
> Signed-off-by: Baoyou Xie <baoyou....@linaro.org>

Thanks.
Acked-by: Yuval Mintz <yuval.mi...@qlogic.com>

RE: [PATCH] qede: mark qede_set_features() static

[Yuval Mintz]

> We get 1 warning when building kernel with W=1:
> drivers/net/ethernet/qlogic/qede/qede_main.c:2113:5: warning: no previous
> prototype for 'qede_set_features' [-Wmissing-prototypes]
> 
> In fact, this function is only used in the file in which it is declared and 
> don't need
> a declaration, but can be made static.
> so this patch marks this function with 'static'.
> 
> Signed-off-by: Baoyou Xie 

Thanks.
Acked-by: Yuval Mintz 

RE: [PATCH] qed: add missing header dependencies

[Yuval Mintz]

> > While I obviously have no strong objection for including
> > qed_selftest.h from qed_selftest.c, I'm not sure I understand which C
> > standard dictates this requirement.
> > Why should a function definition [not call] be preceded by a prototype?
> 
> - When a function is defined in one file and used in another, you want
>   both files to include the same header that has the declaration to
>   ensure that the types are identical. There are cases where the
>   prototype is changed after the fact in an incompatible way, causing
>   silent data corruption on some configurations but maybe not on others.

O.k., motivation is clear.
But this really isn't enforced by the ansi-c standard, right?

Anyway, thanks.

Acked-by: Yuval Mintz <yuval.mi...@qlogic.com>

RE: [PATCH] qed: add missing header dependencies

[Yuval Mintz]

> > While I obviously have no strong objection for including
> > qed_selftest.h from qed_selftest.c, I'm not sure I understand which C
> > standard dictates this requirement.
> > Why should a function definition [not call] be preceded by a prototype?
> 
> - When a function is defined in one file and used in another, you want
>   both files to include the same header that has the declaration to
>   ensure that the types are identical. There are cases where the
>   prototype is changed after the fact in an incompatible way, causing
>   silent data corruption on some configurations but maybe not on others.

O.k., motivation is clear.
But this really isn't enforced by the ansi-c standard, right?

Anyway, thanks.

Acked-by: Yuval Mintz 

RE: [PATCH] qed: mark symbols static where possible

[Yuval Mintz]

> We get a few warnings when building kernel with W=1:
> drivers/net/ethernet/qlogic/qed/qed_l2.c:112:5: warning: no previous
> prototype for 'qed_sp_vport_start' [-Wmissing-prototypes]
> 
> 
> In fact, these functions are only used in the file in which they are
> declared and don't need a declaration, but can be made static.
> so this patch marks these functions with 'static'.
> 
> Signed-off-by: Baoyou Xie 

Most of this changes are correct. I have 2 reservations, though:

1. qed_get_vport_stats() is in use in net-next by qed_main.c starting with
6c75424612a7 ("qed: Add support for NCSI statistics."), so we shouldn't
make it static.
[This raises the question of which repository was this patch built against?]

2. Regarding the changes in qed_cxt.c - some of the functions you're turning
into static are going to be used be other files in our pending RoCE driver
submissions [you could say those ARE really missing their prototypes].
I don't have a real objection to this change - I'm just stating that if you'll
change those now to static, we'll probably 'revert' the change in the near
future.

RE: [PATCH] qed: mark symbols static where possible

[Yuval Mintz]

> We get a few warnings when building kernel with W=1:
> drivers/net/ethernet/qlogic/qed/qed_l2.c:112:5: warning: no previous
> prototype for 'qed_sp_vport_start' [-Wmissing-prototypes]
> 
> 
> In fact, these functions are only used in the file in which they are
> declared and don't need a declaration, but can be made static.
> so this patch marks these functions with 'static'.
> 
> Signed-off-by: Baoyou Xie 

Most of this changes are correct. I have 2 reservations, though:

1. qed_get_vport_stats() is in use in net-next by qed_main.c starting with
6c75424612a7 ("qed: Add support for NCSI statistics."), so we shouldn't
make it static.
[This raises the question of which repository was this patch built against?]

2. Regarding the changes in qed_cxt.c - some of the functions you're turning
into static are going to be used be other files in our pending RoCE driver
submissions [you could say those ARE really missing their prototypes].
I don't have a real objection to this change - I'm just stating that if you'll
change those now to static, we'll probably 'revert' the change in the near
future.

RE: [PATCH] qed: add missing header dependencies

[Yuval Mintz]

> We get 4 warnings when building kernel with W=1:
> drivers/net/ethernet/qlogic/qed/qed_selftest.c:6:5: warning: no previous
> prototype for 'qed_selftest_memory' [-Wmissing-prototypes]
> drivers/net/ethernet/qlogic/qed/qed_selftest.c:19:5: warning: no previous
> prototype for 'qed_selftest_interrupt' [-Wmissing-prototypes]
> drivers/net/ethernet/qlogic/qed/qed_selftest.c:32:5: warning: no previous
> prototype for 'qed_selftest_register' [-Wmissing-prototypes]
> drivers/net/ethernet/qlogic/qed/qed_selftest.c:55:5: warning: no previous
> prototype for 'qed_selftest_clock' [-Wmissing-prototypes]
> 
> In fact, these functions are declared in qed_selftest.h, so this patch add 
> missing
> header dependencies.
> 
> Signed-off-by: Baoyou Xie 

While I obviously have no strong objection for including qed_selftest.h
from qed_selftest.c, I'm not sure I understand which C standard dictates
this requirement.
Why should a function definition [not call] be preceded by a prototype?

RE: [PATCH] qed: add missing header dependencies

[Yuval Mintz]

> We get 4 warnings when building kernel with W=1:
> drivers/net/ethernet/qlogic/qed/qed_selftest.c:6:5: warning: no previous
> prototype for 'qed_selftest_memory' [-Wmissing-prototypes]
> drivers/net/ethernet/qlogic/qed/qed_selftest.c:19:5: warning: no previous
> prototype for 'qed_selftest_interrupt' [-Wmissing-prototypes]
> drivers/net/ethernet/qlogic/qed/qed_selftest.c:32:5: warning: no previous
> prototype for 'qed_selftest_register' [-Wmissing-prototypes]
> drivers/net/ethernet/qlogic/qed/qed_selftest.c:55:5: warning: no previous
> prototype for 'qed_selftest_clock' [-Wmissing-prototypes]
> 
> In fact, these functions are declared in qed_selftest.h, so this patch add 
> missing
> header dependencies.
> 
> Signed-off-by: Baoyou Xie 

While I obviously have no strong objection for including qed_selftest.h
from qed_selftest.c, I'm not sure I understand which C standard dictates
this requirement.
Why should a function definition [not call] be preceded by a prototype?

RE: [PATCH] qed: Remove OOM messages

[Yuval Mintz]

> These messages are unnecessary as OOM allocation failures already do a
> dump_stack() giving more or less the same information.
> 
> $ size drivers/net/ethernet/qlogic/qed/built-in.o* (defconfig x86-64)
>text  data bss dec hex filename
>  126849 27968   32800  187617   2dce1 
> drivers/net/ethernet/qlogic/qed/built-
> in.o.new
>  131506 27968   32800  192274   2ef12 
> drivers/net/ethernet/qlogic/qed/built-
> in.o.old
> 
> Miscellanea:
> 
> o Change allocs to the generally preferred forms where possible.
> 
> Signed-off-by: Joe Perches <j...@perches.com>

Looking good. Thanks Joe.

Acked-by: Yuval Mintz <yuval.mi...@qlogic.com>

RE: [PATCH] qed: Remove OOM messages

[Yuval Mintz]

> These messages are unnecessary as OOM allocation failures already do a
> dump_stack() giving more or less the same information.
> 
> $ size drivers/net/ethernet/qlogic/qed/built-in.o* (defconfig x86-64)
>text  data bss dec hex filename
>  126849 27968   32800  187617   2dce1 
> drivers/net/ethernet/qlogic/qed/built-
> in.o.new
>  131506 27968   32800  192274   2ef12 
> drivers/net/ethernet/qlogic/qed/built-
> in.o.old
> 
> Miscellanea:
> 
> o Change allocs to the generally preferred forms where possible.
> 
> Signed-off-by: Joe Perches 

Looking good. Thanks Joe.

Acked-by: Yuval Mintz 

RE: [PATCH] qed: fix kzalloc-simple.cocci warnings

[Yuval Mintz]

> drivers/net/ethernet/qlogic/qed/qed_dcbx.c:1230:13-20: WARNING: kzalloc
> should be used for dcbx_info, instead of kmalloc/memset
> drivers/net/ethernet/qlogic/qed/qed_dcbx.c:1192:13-20: WARNING: kzalloc
> should be used for dcbx_info, instead of kmalloc/memset
> 
> 
>  Use kzalloc rather than kmalloc followed by memset with 0
> 
>  This considers some simple cases that are common and easy to validate  Note 
> in
> particular that there are no ...s in the rule, so all of the  matched code 
> has to be
> contiguous
> 
> Generated by: scripts/coccinelle/api/alloc/kzalloc-simple.cocci
> 
> CC: Sudarsana Reddy Kalluru <sudarsana.kall...@qlogic.com>
> Signed-off-by: Fengguang Wu <fengguang...@intel.com>

This looks fine; But what's the right process here -
Dave - do we need to re-post this with the the right 'destination' in title
[net/net-next]? Or is it good as-is?
In case of latter,
Acked-by: Yuval Mintz <yuval.mi...@qlogic.com>

One question the automated script -
Can't it [relative] easily be upgraded to also have 'Fixes:' as part of its 
message?

RE: [PATCH] qed: fix kzalloc-simple.cocci warnings

[Yuval Mintz]

> drivers/net/ethernet/qlogic/qed/qed_dcbx.c:1230:13-20: WARNING: kzalloc
> should be used for dcbx_info, instead of kmalloc/memset
> drivers/net/ethernet/qlogic/qed/qed_dcbx.c:1192:13-20: WARNING: kzalloc
> should be used for dcbx_info, instead of kmalloc/memset
> 
> 
>  Use kzalloc rather than kmalloc followed by memset with 0
> 
>  This considers some simple cases that are common and easy to validate  Note 
> in
> particular that there are no ...s in the rule, so all of the  matched code 
> has to be
> contiguous
> 
> Generated by: scripts/coccinelle/api/alloc/kzalloc-simple.cocci
> 
> CC: Sudarsana Reddy Kalluru 
> Signed-off-by: Fengguang Wu 

This looks fine; But what's the right process here -
Dave - do we need to re-post this with the the right 'destination' in title
[net/net-next]? Or is it good as-is?
In case of latter,
Acked-by: Yuval Mintz 

One question the automated script -
Can't it [relative] easily be upgraded to also have 'Fixes:' as part of its 
message?

RE: [PATCH v2 1/4] net: ethernet: ti: davinci_cpdma: split descs num between all channels

[Yuval Mintz]

> Currently the tx channels share same pool of descriptors. Thus one channel can
> block another if pool is emptied by one. But, the shaper should decide which
> channel is allowed to send packets. To avoid such impact of one channel on
> another, let every channel to have its own peace of pool.
Piece.
 
> +/**
> + * cpdma_chan_split_pool - Splits ctrl pool between all channels.
> + * Has to be called under ctlr lock
> + *
> + */
No need for the extra empty comment line.

> + /* calculate average size of pool slice */
> + ch_desc_num = pool->num_desc / ctlr->chan_num;
> +
> + /* split ctlr pool */
> + for (i = 0; i < ARRAY_SIZE(ctlr->channels); i++) {
> + chan = ctlr->channels[i];
> + if (chan)
> + chan->desc_num = ch_desc_num;
Is this 'if' needed? If there's some route where the channel can be NULL,
You're splitting the value incorrectly.

RE: [PATCH v2 1/4] net: ethernet: ti: davinci_cpdma: split descs num between all channels

[Yuval Mintz]

> Currently the tx channels share same pool of descriptors. Thus one channel can
> block another if pool is emptied by one. But, the shaper should decide which
> channel is allowed to send packets. To avoid such impact of one channel on
> another, let every channel to have its own peace of pool.
Piece.
 
> +/**
> + * cpdma_chan_split_pool - Splits ctrl pool between all channels.
> + * Has to be called under ctlr lock
> + *
> + */
No need for the extra empty comment line.

> + /* calculate average size of pool slice */
> + ch_desc_num = pool->num_desc / ctlr->chan_num;
> +
> + /* split ctlr pool */
> + for (i = 0; i < ARRAY_SIZE(ctlr->channels); i++) {
> + chan = ctlr->channels[i];
> + if (chan)
> + chan->desc_num = ch_desc_num;
Is this 'if' needed? If there's some route where the channel can be NULL,
You're splitting the value incorrectly.

RE: [PATCH 04/21] net: thunderx: Set queue count based on number of CPUs

[Yuval Mintz]

> 81xx has only 4 CPUs, so it doesn't make sense to initialize entire Qset i.e 8
> queues by default. Made changes to queue initialization to init queues equal 
> to
> number of CPUs or
> 8 queues whichever is lesser. Also this will be applicable to VMs with VNIC VF
> attached and having less VCPUs
...
> - qcount = MAX_CMP_QUEUES_PER_QS;
> + qcount = min_t(int, MAX_CMP_QUEUES_PER_QS, num_online_cpus());

We have netif_get_num_default_rss_queues() which does this.

RE: [PATCH 04/21] net: thunderx: Set queue count based on number of CPUs

[Yuval Mintz]

> 81xx has only 4 CPUs, so it doesn't make sense to initialize entire Qset i.e 8
> queues by default. Made changes to queue initialization to init queues equal 
> to
> number of CPUs or
> 8 queues whichever is lesser. Also this will be applicable to VMs with VNIC VF
> attached and having less VCPUs
...
> - qcount = MAX_CMP_QUEUES_PER_QS;
> + qcount = min_t(int, MAX_CMP_QUEUES_PER_QS, num_online_cpus());

We have netif_get_num_default_rss_queues() which does this.

RE: [PATCH net 2/4] hv_netvsc: reset vf_inject on VF removal

[Yuval Mintz]

> +static void netvsc_inject_enable(struct net_device_context
> +*net_device_ctx) {
> + net_device_ctx->vf_inject = true;
> +}
> +
> +static void netvsc_inject_disable(struct net_device_context
> +*net_device_ctx) {
> + net_device_ctx->vf_inject = false;
> +
> + /* Wait for currently active users to drain out. */
> + while (atomic_read(_device_ctx->vf_use_cnt) != 0)
> + udelay(50);
> +}

That was already the behavior before, but are you certain you
want to unconditionally block without any possible timeout?

RE: [PATCH net 2/4] hv_netvsc: reset vf_inject on VF removal

[Yuval Mintz]

> +static void netvsc_inject_enable(struct net_device_context
> +*net_device_ctx) {
> + net_device_ctx->vf_inject = true;
> +}
> +
> +static void netvsc_inject_disable(struct net_device_context
> +*net_device_ctx) {
> + net_device_ctx->vf_inject = false;
> +
> + /* Wait for currently active users to drain out. */
> + while (atomic_read(_device_ctx->vf_use_cnt) != 0)
> + udelay(50);
> +}

That was already the behavior before, but are you certain you
want to unconditionally block without any possible timeout?

RE: [PATCH 1/1] qed: do not use unitialized variable

[Yuval Mintz]

> Do not write random bytes from the kernel stack when calling qed_wr.
> 
> Signed-off-by: Heinrich Schuchardt <xypron.g...@gmx.de>

Thanks.

Acked-by: Yuval Mintz <yuval.mi...@qlogic.com>

RE: [PATCH 1/1] qed: do not use unitialized variable

[Yuval Mintz]

> Do not write random bytes from the kernel stack when calling qed_wr.
> 
> Signed-off-by: Heinrich Schuchardt 

Thanks.

Acked-by: Yuval Mintz 

RE: [PATCH] qed: Add and use specific logging functions to reduce object size

[Yuval Mintz]

> > Current DP_ macros generate a lot of code.
> > Using functions with vsprintf extension %pV helps reduce that size.
> 
> Yuval, I used the same KERN_ output types, but it is unusual
> that DP_INFO outputs at KERN_NOTICE.
> 
> Was that a copy/paste defect or should it be emitted at KERN_INFO and
> DP_VERBOSE be emitted at KERN_DEBUG?

I agree it's a bit odd, but it's actually by design - 
as none of these prints would ever be reached unless user explicitly enable
them [ethtool/module param], the assumption is that NOTICE is good
enough, i.e., it will prevent the need for doing additional configuration of
logging levels by the user.

RE: [PATCH] qed: Add and use specific logging functions to reduce object size

[Yuval Mintz]

> > Current DP_ macros generate a lot of code.
> > Using functions with vsprintf extension %pV helps reduce that size.
> 
> Yuval, I used the same KERN_ output types, but it is unusual
> that DP_INFO outputs at KERN_NOTICE.
> 
> Was that a copy/paste defect or should it be emitted at KERN_INFO and
> DP_VERBOSE be emitted at KERN_DEBUG?

I agree it's a bit odd, but it's actually by design - 
as none of these prints would ever be reached unless user explicitly enable
them [ethtool/module param], the assumption is that NOTICE is good
enough, i.e., it will prevent the need for doing additional configuration of
logging levels by the user.

RE: [PATCH] qed: Add and use specific logging functions to reduce object size

[Yuval Mintz]

> Current DP_ macros generate a lot of code.
> Using functions with vsprintf extension %pV helps reduce that size.
> 
>  drivers/net/ethernet/qlogic/qed/Makefile   |  2 +-
>  drivers/net/ethernet/qlogic/qed/qed_util.c | 82
> ++
>  include/linux/qed/qed_if.h | 60 +-
>  3 files changed, 106 insertions(+), 38 deletions(-)  create mode 100644
> drivers/net/ethernet/qlogic/qed/qed_util.c

This won't compile when CONFIG_QED*=m, as qede can't link to
the new qed_{err, verbose, info, notice} functions.
That was the original reason for putting the macros in the interface
header.

Other alternatives:
- We can EXPORT_SYMBOL() the functions, although we've taken a
strain not adding such to the interface.
 - Code duplication between qed/qede [ugly].
 - Implementing this in qede via the interface functions with qed; 
But the notion of defining an interface between 2 modules passing
va_args seems [to me] like a bad one.

If you have cleaner solutions, I'd be happy to hear those.

RE: [PATCH] qed: Add and use specific logging functions to reduce object size

[Yuval Mintz]

> Current DP_ macros generate a lot of code.
> Using functions with vsprintf extension %pV helps reduce that size.
> 
>  drivers/net/ethernet/qlogic/qed/Makefile   |  2 +-
>  drivers/net/ethernet/qlogic/qed/qed_util.c | 82
> ++
>  include/linux/qed/qed_if.h | 60 +-
>  3 files changed, 106 insertions(+), 38 deletions(-)  create mode 100644
> drivers/net/ethernet/qlogic/qed/qed_util.c

This won't compile when CONFIG_QED*=m, as qede can't link to
the new qed_{err, verbose, info, notice} functions.
That was the original reason for putting the macros in the interface
header.

Other alternatives:
- We can EXPORT_SYMBOL() the functions, although we've taken a
strain not adding such to the interface.
 - Code duplication between qed/qede [ugly].
 - Implementing this in qede via the interface functions with qed; 
But the notion of defining an interface between 2 modules passing
va_args seems [to me] like a bad one.

If you have cleaner solutions, I'd be happy to hear those.

RE: [PATCH v2] qed: fix qed_fill_link() error handling

[Yuval Mintz]

> gcc warns about qed_fill_link possibly accessing uninitialized data:
> 
> drivers/net/ethernet/qlogic/qed/qed_main.c: In function 'qed_fill_link':
> drivers/net/ethernet/qlogic/qed/qed_main.c:1170:35: error: 'link_caps' may be
> used uninitialized in this function [-Werror=maybe-uninitialized]
> 
> While this warning is only about the specific case of CONFIG_QED_SRIOV being
> disabled but the function getting called for a VF (which should never happen),
> another possibility is that qed_mcp_get_*() fails without returning data.
> 
> This rearranges the code so we bail out in either of the two cases and print a
> warning instead of accessing the uninitialized data.
> 
> The qed_link_output structure remains untouched in this case, but all callers 
> first
> call memset() on it, so at least we are not leaking stack data then.
> 
> As discussed, we also use a compile-time check to ensure we never use any of
> the VF code if CONFIG_QED_SRIOV is disabled, and the PCI device table is
> updated to no longer bind to virtual functions in that configuration.
> 
> Signed-off-by: Arnd Bergmann <a...@arndb.de>
> 
> ---
> On Wednesday, June 1, 2016 11:10:30 AM CEST Yuval Mintz wrote:
> > Actually, I think VF probe should gracefully fail in that case, as
> > qed_vf_hw_prepare() would simply return -EINVAL.
> > But I can honestly say I've never tested this flow, and I agree
> > there's no reason to allow VF probe in case we're not supporting SRIOV.
> 
> ok
> 
> > So I guess removing the PCI ID and defining IS_PF to be true in case
> > CONFIG_QED_SRIOV isn't set is the right way to go.
> > Do you want to revise your patch, or do you want me to do it?
> 
> I've done the patch below now, please either Ack or modify it the way you like
> and forward it.
> 
> Thanks,
> 
>   Arnd

Perhaps it would have made more sense as a 2-part series; But I'm content with
the changes themselves. I'd let Dave decide whether he wants it split.

Thanks for taking the time fixing this.

Acked-by: Yuval Mintz <yuval.mi...@qlogic.com>

RE: [PATCH v2] qed: fix qed_fill_link() error handling

[Yuval Mintz]

> gcc warns about qed_fill_link possibly accessing uninitialized data:
> 
> drivers/net/ethernet/qlogic/qed/qed_main.c: In function 'qed_fill_link':
> drivers/net/ethernet/qlogic/qed/qed_main.c:1170:35: error: 'link_caps' may be
> used uninitialized in this function [-Werror=maybe-uninitialized]
> 
> While this warning is only about the specific case of CONFIG_QED_SRIOV being
> disabled but the function getting called for a VF (which should never happen),
> another possibility is that qed_mcp_get_*() fails without returning data.
> 
> This rearranges the code so we bail out in either of the two cases and print a
> warning instead of accessing the uninitialized data.
> 
> The qed_link_output structure remains untouched in this case, but all callers 
> first
> call memset() on it, so at least we are not leaking stack data then.
> 
> As discussed, we also use a compile-time check to ensure we never use any of
> the VF code if CONFIG_QED_SRIOV is disabled, and the PCI device table is
> updated to no longer bind to virtual functions in that configuration.
> 
> Signed-off-by: Arnd Bergmann 
> 
> ---
> On Wednesday, June 1, 2016 11:10:30 AM CEST Yuval Mintz wrote:
> > Actually, I think VF probe should gracefully fail in that case, as
> > qed_vf_hw_prepare() would simply return -EINVAL.
> > But I can honestly say I've never tested this flow, and I agree
> > there's no reason to allow VF probe in case we're not supporting SRIOV.
> 
> ok
> 
> > So I guess removing the PCI ID and defining IS_PF to be true in case
> > CONFIG_QED_SRIOV isn't set is the right way to go.
> > Do you want to revise your patch, or do you want me to do it?
> 
> I've done the patch below now, please either Ack or modify it the way you like
> and forward it.
> 
> Thanks,
> 
>   Arnd

Perhaps it would have made more sense as a 2-part series; But I'm content with
the changes themselves. I'd let Dave decide whether he wants it split.

Thanks for taking the time fixing this.

Acked-by: Yuval Mintz 

RE: [PATCH] qed: fix qed_fill_link() error handling

[Yuval Mintz]

> > > I think we can just remove the IS_ENABLED() check there and define
> > > the
> > > IS_PF() macro conditionally to become 'true' if CONFIG_QED_SRIOV is
> > > not set, like some other drivers do
> >
> > I think that would be unsafe with current qede - qede currently
> > publishes its VFs' PCI device-id as part its MODULE_DEVICE_TABLE, even
> > if CONFIG_QED_SRIOV isn't enabled [might be the wrong thing to do, but
> > that how it goes].
> > Without changing this, if for some reason we'd have an assigned VF to
> > a VM whose kernel isn't compiled with CONFIG_QED_SRIOV [which is an
> > odd config], that VM is likely to miserably crash.
> 
> Wouldn't it crash anyway if the code to handle VF devices is not present?
> E.g. the warning we got here tells us that qed_get_link_data() operates on
> uninitialized data when called on a VF device and SRIOV support is not built 
> into
> the driver. I haven't looked if all the other functions handle that right, 
> but my
> guess is that there are other functions with similar problems.
> 
> Maybe it's best to remove the PCI IDs fort the virtual devices from the table 
> if
> they are not supported by the configuration.

Actually, I think VF probe should gracefully fail in that case,
as qed_vf_hw_prepare() would simply return -EINVAL.
But I can honestly say I've never tested this flow, and I agree there's
no reason to allow VF probe in case we're not supporting SRIOV.

So I guess removing the PCI ID and defining IS_PF to be true in case
CONFIG_QED_SRIOV isn't set is the right way to go.
Do you want to revise your patch, or do you want me to do it?

Thanks,
Yuval

RE: [PATCH] qed: fix qed_fill_link() error handling

[Yuval Mintz]

> > > I think we can just remove the IS_ENABLED() check there and define
> > > the
> > > IS_PF() macro conditionally to become 'true' if CONFIG_QED_SRIOV is
> > > not set, like some other drivers do
> >
> > I think that would be unsafe with current qede - qede currently
> > publishes its VFs' PCI device-id as part its MODULE_DEVICE_TABLE, even
> > if CONFIG_QED_SRIOV isn't enabled [might be the wrong thing to do, but
> > that how it goes].
> > Without changing this, if for some reason we'd have an assigned VF to
> > a VM whose kernel isn't compiled with CONFIG_QED_SRIOV [which is an
> > odd config], that VM is likely to miserably crash.
> 
> Wouldn't it crash anyway if the code to handle VF devices is not present?
> E.g. the warning we got here tells us that qed_get_link_data() operates on
> uninitialized data when called on a VF device and SRIOV support is not built 
> into
> the driver. I haven't looked if all the other functions handle that right, 
> but my
> guess is that there are other functions with similar problems.
> 
> Maybe it's best to remove the PCI IDs fort the virtual devices from the table 
> if
> they are not supported by the configuration.

Actually, I think VF probe should gracefully fail in that case,
as qed_vf_hw_prepare() would simply return -EINVAL.
But I can honestly say I've never tested this flow, and I agree there's
no reason to allow VF probe in case we're not supporting SRIOV.

So I guess removing the PCI ID and defining IS_PF to be true in case
CONFIG_QED_SRIOV isn't set is the right way to go.
Do you want to revise your patch, or do you want me to do it?

Thanks,
Yuval

RE: [PATCH] qed: fix qed_fill_link() error handling

[Yuval Mintz]

> > I think both solutions are equally valid/elegant.
> >
> > Arnd?
> 
> I think we can just remove the IS_ENABLED() check there and define the
> IS_PF() macro conditionally to become 'true' if CONFIG_QED_SRIOV is not set,
> like some other drivers do
> 
> diff --git a/drivers/net/ethernet/qlogic/qed/qed_main.c
> b/drivers/net/ethernet/qlogic/qed/qed_main.c
> index 287f61c20c19..756176525cf9 100644
> --- a/drivers/net/ethernet/qlogic/qed/qed_main.c
> +++ b/drivers/net/ethernet/qlogic/qed/qed_main.c
> @@ -1110,7 +1110,7 @@ static int qed_get_link_data(struct qed_hwfn *hwfn,
> {
>   void *p;
> 
> - if (IS_ENABLED(CONFIG_QED_SRIOV) && !IS_PF(hwfn->cdev)) {
> + if (!IS_PF(hwfn->cdev)) {
>   qed_vf_get_link_params(hwfn, params);
>   qed_vf_get_link_state(hwfn, link);
>   qed_vf_get_link_caps(hwfn, link_caps); diff --git
> a/drivers/net/ethernet/qlogic/qed/qed_sriov.h
> b/drivers/net/ethernet/qlogic/qed/qed_sriov.h
> index c8667c65e685..c90b2b6ad969 100644
> --- a/drivers/net/ethernet/qlogic/qed/qed_sriov.h
> +++ b/drivers/net/ethernet/qlogic/qed/qed_sriov.h
> @@ -12,11 +12,13 @@
>  #include "qed_vf.h"
>  #define QED_VF_ARRAY_LENGTH (3)
> 
> +#ifdef CONFIG_QED_SRIOV
>  #define IS_VF(cdev) ((cdev)->b_is_vf)
>  #define IS_PF(cdev) (!((cdev)->b_is_vf))
> -#ifdef CONFIG_QED_SRIOV
>  #define IS_PF_SRIOV(p_hwfn) (!!((p_hwfn)->cdev->p_iov_info))
>  #else
> +#define IS_VF(cdev) (0)
> +#define IS_PF(cdev) (1)
>  #define IS_PF_SRIOV(p_hwfn) (0)
>  #endif
>  #define IS_PF_SRIOV_ALLOC(p_hwfn)   (!!((p_hwfn)->pf_iov_info))
> 
> I don't see why that isn't already the case actually. If this is ok, I'll 
> send an
> updated patch.
> 
> For the PF case, we still need to fix the qed_mcp_get_link_params() failure 
> case,
> so the rest of my patch is needed anyway, regardless of how we address the
> warning.
> 
>   Arnd

I think that would be unsafe with current qede - 
qede currently publishes its VFs' PCI device-id as part its MODULE_DEVICE_TABLE,
even if CONFIG_QED_SRIOV isn't enabled [might be the wrong thing to do, but that
how it goes].
Without changing this, if for some reason we'd have an assigned VF to a VM
whose kernel isn't compiled with CONFIG_QED_SRIOV [which is an odd config],
that VM is likely to miserably crash.

RE: [PATCH] qed: fix qed_fill_link() error handling

[Yuval Mintz]

> > I think both solutions are equally valid/elegant.
> >
> > Arnd?
> 
> I think we can just remove the IS_ENABLED() check there and define the
> IS_PF() macro conditionally to become 'true' if CONFIG_QED_SRIOV is not set,
> like some other drivers do
> 
> diff --git a/drivers/net/ethernet/qlogic/qed/qed_main.c
> b/drivers/net/ethernet/qlogic/qed/qed_main.c
> index 287f61c20c19..756176525cf9 100644
> --- a/drivers/net/ethernet/qlogic/qed/qed_main.c
> +++ b/drivers/net/ethernet/qlogic/qed/qed_main.c
> @@ -1110,7 +1110,7 @@ static int qed_get_link_data(struct qed_hwfn *hwfn,
> {
>   void *p;
> 
> - if (IS_ENABLED(CONFIG_QED_SRIOV) && !IS_PF(hwfn->cdev)) {
> + if (!IS_PF(hwfn->cdev)) {
>   qed_vf_get_link_params(hwfn, params);
>   qed_vf_get_link_state(hwfn, link);
>   qed_vf_get_link_caps(hwfn, link_caps); diff --git
> a/drivers/net/ethernet/qlogic/qed/qed_sriov.h
> b/drivers/net/ethernet/qlogic/qed/qed_sriov.h
> index c8667c65e685..c90b2b6ad969 100644
> --- a/drivers/net/ethernet/qlogic/qed/qed_sriov.h
> +++ b/drivers/net/ethernet/qlogic/qed/qed_sriov.h
> @@ -12,11 +12,13 @@
>  #include "qed_vf.h"
>  #define QED_VF_ARRAY_LENGTH (3)
> 
> +#ifdef CONFIG_QED_SRIOV
>  #define IS_VF(cdev) ((cdev)->b_is_vf)
>  #define IS_PF(cdev) (!((cdev)->b_is_vf))
> -#ifdef CONFIG_QED_SRIOV
>  #define IS_PF_SRIOV(p_hwfn) (!!((p_hwfn)->cdev->p_iov_info))
>  #else
> +#define IS_VF(cdev) (0)
> +#define IS_PF(cdev) (1)
>  #define IS_PF_SRIOV(p_hwfn) (0)
>  #endif
>  #define IS_PF_SRIOV_ALLOC(p_hwfn)   (!!((p_hwfn)->pf_iov_info))
> 
> I don't see why that isn't already the case actually. If this is ok, I'll 
> send an
> updated patch.
> 
> For the PF case, we still need to fix the qed_mcp_get_link_params() failure 
> case,
> so the rest of my patch is needed anyway, regardless of how we address the
> warning.
> 
>   Arnd

I think that would be unsafe with current qede - 
qede currently publishes its VFs' PCI device-id as part its MODULE_DEVICE_TABLE,
even if CONFIG_QED_SRIOV isn't enabled [might be the wrong thing to do, but that
how it goes].
Without changing this, if for some reason we'd have an assigned VF to a VM
whose kernel isn't compiled with CONFIG_QED_SRIOV [which is an odd config],
that VM is likely to miserably crash.

RE: [PATCH net] bnx2x: avoid leaking memory on bnx2x_init_one() failures

[Yuval Mintz]

> bnx2x_init_bp() allocates memory with bnx2x_alloc_mem_bp() so if we fail later
> in bnx2x_init_one() we need to free this memory with bnx2x_free_mem_bp() to
> avoid leakages. E.g. I'm observing memory leaks reported by kmemleak when a
> failure (unrelated) happens in bnx2x_vfpf_acquire().
> 
> Signed-off-by: Vitaly Kuznetsov <vkuzn...@redhat.com>

Thanks.

Acked-by: Yuval Mintz <yuval.mi...@qlogic.com>

RE: [PATCH net] bnx2x: avoid leaking memory on bnx2x_init_one() failures

[Yuval Mintz]

> bnx2x_init_bp() allocates memory with bnx2x_alloc_mem_bp() so if we fail later
> in bnx2x_init_one() we need to free this memory with bnx2x_free_mem_bp() to
> avoid leakages. E.g. I'm observing memory leaks reported by kmemleak when a
> failure (unrelated) happens in bnx2x_vfpf_acquire().
> 
> Signed-off-by: Vitaly Kuznetsov 

Thanks.

Acked-by: Yuval Mintz 

RE: [PATCH] qed: fix qed_fill_link() error handling

[Yuval Mintz]

> + if (IS_ENABLED(CONFIG_QED_SRIOV) && !IS_PF(hwfn->cdev)) {
> + qed_vf_get_link_params(hwfn, params);
> + qed_vf_get_link_state(hwfn, link);
> + qed_vf_get_link_caps(hwfn, link_caps);
> +
> + return 0;
> + }

The IS_ENABLED here seems a bit wasteful to me - we have empty implementation
under qed_vf.h just for this case [I.e., that SRIOV isn't enabled for qed].
If all we're trying achieve is removing these gcc warnings, I think we can 
simply
memset the structs in the currently-empty qed_vf_get_link_* functions.

RE: [PATCH] qed: fix qed_fill_link() error handling

[Yuval Mintz]

> + if (IS_ENABLED(CONFIG_QED_SRIOV) && !IS_PF(hwfn->cdev)) {
> + qed_vf_get_link_params(hwfn, params);
> + qed_vf_get_link_state(hwfn, link);
> + qed_vf_get_link_caps(hwfn, link_caps);
> +
> + return 0;
> + }

The IS_ENABLED here seems a bit wasteful to me - we have empty implementation
under qed_vf.h just for this case [I.e., that SRIOV isn't enabled for qed].
If all we're trying achieve is removing these gcc warnings, I think we can 
simply
memset the structs in the currently-empty qed_vf_get_link_* functions.

RE: drivers/net/ethernet/qlogic/qed/qed_dcbx.c:210: pointless test ?

[Yuval Mintz]

> Hello there,
> 
> drivers/net/ethernet/qlogic/qed/qed_dcbx.c:210:16: warning: comparison is
> always false due to limited range of data type [-Wtype-limits]
> 
> Source code is
> 
>if (priority < 0) {
> 
> but
> 
> u8 tc, priority, priority_map;
> 
> 
> Regards
> 
> David Binderman

Hi David,

Dan Carpenter has already sent a fix for this one ~an hour ago.
[https://patchwork.ozlabs.org/patch/625114/]

Thanks,
Yuval

RE: drivers/net/ethernet/qlogic/qed/qed_dcbx.c:210: pointless test ?

[Yuval Mintz]

> Hello there,
> 
> drivers/net/ethernet/qlogic/qed/qed_dcbx.c:210:16: warning: comparison is
> always false due to limited range of data type [-Wtype-limits]
> 
> Source code is
> 
>if (priority < 0) {
> 
> but
> 
> u8 tc, priority, priority_map;
> 
> 
> Regards
> 
> David Binderman

Hi David,

Dan Carpenter has already sent a fix for this one ~an hour ago.
[https://patchwork.ozlabs.org/patch/625114/]

Thanks,
Yuval

RE: bnx2x in 4.6rc7 with FW 7.13.1.0 not present

[Yuval Mintz]

> Upgrading a system from kernel 4.2 to 4.6rc7, there is an extra 2 minute delay
> while booting due to these problems:
> 
> [   47.977221] bnx2x :04:00.1: Direct firmware load for 
> bnx2x/bnx2x-e2-
> 7.13.1.0.fw failed with error -2
... 
> Could the driver fall back to an older firmware version more gracefully?

No.

The internal FW isn't necessarily backward compatible [there are some
Compatibility in fastpath in order to support VFs, but not in general].
Doing what you ask would means filling the driver with a lot of legacy code
for older FWs, adding per-FW version HSI files, etc.

So in short - if you've installed a fresh kernel on top of an existing distro
that lacked the latest firmware in its filesystem, getting that missing 
.bin file from linux-firmware is the way to go.

Cheers,
Yuval

RE: bnx2x in 4.6rc7 with FW 7.13.1.0 not present

[Yuval Mintz]

> Upgrading a system from kernel 4.2 to 4.6rc7, there is an extra 2 minute delay
> while booting due to these problems:
> 
> [   47.977221] bnx2x :04:00.1: Direct firmware load for 
> bnx2x/bnx2x-e2-
> 7.13.1.0.fw failed with error -2
... 
> Could the driver fall back to an older firmware version more gracefully?

No.

The internal FW isn't necessarily backward compatible [there are some
Compatibility in fastpath in order to support VFs, but not in general].
Doing what you ask would means filling the driver with a lot of legacy code
for older FWs, adding per-FW version HSI files, etc.

So in short - if you've installed a fresh kernel on top of an existing distro
that lacked the latest firmware in its filesystem, getting that missing 
.bin file from linux-firmware is the way to go.

Cheers,
Yuval

RE: [PATCH] qed: initialize return rc to avoid returning garbage

[Yuval Mintz]

> From: Colin Ian King <colin.k...@canonical.com>
> 
> in the case where qed_slowpath_irq_req is not called, rc is not assigned and 
> so
> qed_int_igu_enable will return a garbage value.
> Fix this by initializing rc to 0.
> 
> Signed-off-by: Colin Ian King <colin.k...@canonical.com>

Thanks Colin.
Acked-by: Yuval Mintz <yuval.mi...@qlogic.com>

RE: [PATCH] qed: initialize return rc to avoid returning garbage

[Yuval Mintz]

> From: Colin Ian King 
> 
> in the case where qed_slowpath_irq_req is not called, rc is not assigned and 
> so
> qed_int_igu_enable will return a garbage value.
> Fix this by initializing rc to 0.
> 
> Signed-off-by: Colin Ian King 

Thanks Colin.
Acked-by: Yuval Mintz 

Re: [PATCH] bnx2x: add a separate GENEVE Kconfig symbol

[Yuval Mintz]

> When CONFIG_GENEVE is built as a loadable module, and bnx2x is built-in,
> we get this link error:

> drivers/net/built-in.o: In function `bnx2x_open':
> :(.text+0x33322): undefined reference to `geneve_get_rx_port'
> drivers/net/built-in.o: In function `bnx2x_sp_rtnl_task':
> :(.text+0x3e632): undefined reference to `geneve_get_rx_port'

> This avoids the problem by adding a separate Kconfig symbol named
> CONFIG_BNX2X_GENEVE that is only enabled when the code is
> reachable from the driver.

> This is the same trick that BNX2X does for VXLAN support, and
> is similar to how I40E handles both.

> Signed-off-by: Arnd Bergmann <a...@arndb.de>
> Fixes: 883ce97d25b0 ("bnx2x: Add Geneve inner-RSS support")

Ouch. Thanks for fixing this one; I tried avoiding this solution
since I don't think there's any reason to encumber the config
file with needless options [I.e., what's the gain in the ability to
disable a single offloaded protocol on a single network driver?],
but obviously, I botched the job.

BTW, if you want to handle this exactly like vxlan, you might
consider using `defined(CONFIG_BNX2X_GENEVE)' instead of
`IS_ENABLED(CONFIG_BNX2X_GENEVE)'  [It not a tristate].
But that's truly insignificant.

Acked-By: Yuval Mintz <yuval.mi...@qlogic.com>

Re: [PATCH] bnx2x: add a separate GENEVE Kconfig symbol

[Yuval Mintz]

> When CONFIG_GENEVE is built as a loadable module, and bnx2x is built-in,
> we get this link error:

> drivers/net/built-in.o: In function `bnx2x_open':
> :(.text+0x33322): undefined reference to `geneve_get_rx_port'
> drivers/net/built-in.o: In function `bnx2x_sp_rtnl_task':
> :(.text+0x3e632): undefined reference to `geneve_get_rx_port'

> This avoids the problem by adding a separate Kconfig symbol named
> CONFIG_BNX2X_GENEVE that is only enabled when the code is
> reachable from the driver.

> This is the same trick that BNX2X does for VXLAN support, and
> is similar to how I40E handles both.

> Signed-off-by: Arnd Bergmann 
> Fixes: 883ce97d25b0 ("bnx2x: Add Geneve inner-RSS support")

Ouch. Thanks for fixing this one; I tried avoiding this solution
since I don't think there's any reason to encumber the config
file with needless options [I.e., what's the gain in the ability to
disable a single offloaded protocol on a single network driver?],
but obviously, I botched the job.

BTW, if you want to handle this exactly like vxlan, you might
consider using `defined(CONFIG_BNX2X_GENEVE)' instead of
`IS_ENABLED(CONFIG_BNX2X_GENEVE)'  [It not a tristate].
But that's truly insignificant.

Acked-By: Yuval Mintz 

RE: linux-next: build failure after merge of the net-next tree

[Yuval Mintz]

> > After merging the net-next tree, today's linux-next build (powerpc
> > ppc64_defconfig) failed like this:
> >
> > In file included from drivers/net/ethernet/broadcom/bnx2x/bnx2x.h:56:0,
> >  from drivers/net/ethernet/broadcom/bnx2x/bnx2x_dcb.c:30:
> > drivers/net/ethernet/broadcom/bnx2x/bnx2x_dcb.c: In function
> 'bnx2x_dcbx_get_ap_feature':
> > drivers/net/ethernet/broadcom/bnx2x/bnx2x_dcb.c:224:11: error:
> 'DCBX_APP_SF_DEFAULT' undeclared (first use in this function)
> >DCBX_APP_SF_DEFAULT) &&
> >^
> > Caused by commit
> >
> >   e5d3a51cefbb ("This adds support for default application priority.")
> >
> > This build is big endian.
> 
> Yuval and Ariel, you _MUST_ fix this.
> 
> This build failure has been in the tree for 24 hours and I haven't heard 
> anything
> from you two yet.

Hi Dave,

Perhaps I wasn't clear enough in the title I've provided for the fixing patch,
but I've sent it yesterday and it's marked as 'under review' for net-next.
The patch is "bnx2x: Add missing HSI for big-endian machines".

Sorry about all the noise.

RE: linux-next: build failure after merge of the net-next tree

[Yuval Mintz]

> > After merging the net-next tree, today's linux-next build (powerpc
> > ppc64_defconfig) failed like this:
> >
> > In file included from drivers/net/ethernet/broadcom/bnx2x/bnx2x.h:56:0,
> >  from drivers/net/ethernet/broadcom/bnx2x/bnx2x_dcb.c:30:
> > drivers/net/ethernet/broadcom/bnx2x/bnx2x_dcb.c: In function
> 'bnx2x_dcbx_get_ap_feature':
> > drivers/net/ethernet/broadcom/bnx2x/bnx2x_dcb.c:224:11: error:
> 'DCBX_APP_SF_DEFAULT' undeclared (first use in this function)
> >DCBX_APP_SF_DEFAULT) &&
> >^
> > Caused by commit
> >
> >   e5d3a51cefbb ("This adds support for default application priority.")
> >
> > This build is big endian.
> 
> Yuval and Ariel, you _MUST_ fix this.
> 
> This build failure has been in the tree for 24 hours and I haven't heard 
> anything
> from you two yet.

Hi Dave,

Perhaps I wasn't clear enough in the title I've provided for the fixing patch,
but I've sent it yesterday and it's marked as 'under review' for net-next.
The patch is "bnx2x: Add missing HSI for big-endian machines".

Sorry about all the noise.

RE: [PATCH V4 net-next 4/5] net:hns: Add support of ethtool TSO set option for Hip06 in HNS

[Yuval Mintz]

> > Isn't AE_VERSION_1 something fixed once you publish your features?
> > If it can't be changed, why not simply remove the features from
> > `hw_features' instead of having to implement this ndo?
> There could be a case where the feature is supported by the SoC
> and therefore it is already part of the 'hw_features' but it has been
> say DISABLED or ENABLED by ethtool. In such a case, we need to
> make sure we strike off that feature from the 'features' flag.
> 
> Therefore, we need this leg I suppose. Let me know If I am missing
> something here or there is a gap in my understanding.

Look at ethtool_set_features() - if something isn't listed in hw_features,
you're not support to be able to change it.

If you can make a one-time decision when registering the netdevice
whether this feature is supported by HW or not [assuming AE_VERSION_1
is available at that point and can't later change] than it's the better 
alternative.

Cheers,
Yuval 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH V4 net-next 4/5] net:hns: Add support of ethtool TSO set option for Hip06 in HNS

[Yuval Mintz]

> > Isn't AE_VERSION_1 something fixed once you publish your features?
> > If it can't be changed, why not simply remove the features from
> > `hw_features' instead of having to implement this ndo?
> There could be a case where the feature is supported by the SoC
> and therefore it is already part of the 'hw_features' but it has been
> say DISABLED or ENABLED by ethtool. In such a case, we need to
> make sure we strike off that feature from the 'features' flag.
> 
> Therefore, we need this leg I suppose. Let me know If I am missing
> something here or there is a gap in my understanding.

Look at ethtool_set_features() - if something isn't listed in hw_features,
you're not support to be able to change it.

If you can make a one-time decision when registering the netdevice
whether this feature is supported by HW or not [assuming AE_VERSION_1
is available at that point and can't later change] than it's the better 
alternative.

Cheers,
Yuval 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH V4 net-next 4/5] net:hns: Add support of ethtool TSO set option for Hip06 in HNS

[Yuval Mintz]

> +static netdev_features_t hns_nic_fix_features(
> + struct net_device *netdev, netdev_features_t features) {
> + struct hns_nic_priv *priv = netdev_priv(netdev);
> +
> + switch (priv->enet_ver) {
> + case AE_VERSION_1:
> + features &= ~(NETIF_F_TSO | NETIF_F_TSO6 |
> + NETIF_F_HW_VLAN_CTAG_FILTER);
> + break;
> + default:
> + break;
> + }
> + return features;
> +}
> +

Isn't AE_VERSION_1 something fixed once you publish your features?
If it can't be changed, why not simply remove the features from
`hw_features' instead of having to implement this ndo?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH V3 net-next 2/5] net:hns: Add Hip06 "RSS(Receive Side Scaling)" support to HNS Driver

[Yuval Mintz]

>  static void hns_ppe_init_hw(struct hns_ppe_cb *ppe_cb)  {
...
> + /* Set default RSS key and indrection table*/
> + const u32 rss_key[HNS_PPEV2_RSS_KEY_NUM] = {
> + 0x6d5a56da, 0x255b0ec2,
> + 0x4167253d, 0x43a38fb0,
> + 0xd0ca2bcb, 0xae7b30b4,
> + 0x77cb2da3, 0x8030f20c,
> + 0x6a42b73b, 0xbeac01fa,
> + };
> +
> + /* set default RSS key and remember it */
> + for (i = 0; i < HNS_PPEV2_RSS_KEY_NUM; i++)
> + ppe_cb->rss_key[i]  = rss_key[i];
> 

Is there any reason for the special default key?
Why not use netdev_rss_key_fill()?

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH V3 net-next 1/5] net:hns: Add support of Hip06 SoC to the Hislicon Network Subsystem

[Yuval Mintz]

> +void hns_rcbv2_int_ctrl_hw(struct hnae_queue *q, u32 flag, u32 mask)
> +{
> + u32 int_mask_en = !!mask;
> +
> + if (flag & RCB_INT_FLAG_TX)
> + dsaf_write_dev(q, RCB_RING_INTMSK_TXWL_REG,
> int_mask_en);
> +
> + if (flag & RCB_INT_FLAG_RX)
> + dsaf_write_dev(q, RCB_RING_INTMSK_RXWL_REG,
> int_mask_en);
> +}
> +
> +void hns_rcbv2_int_clr_hw(struct hnae_queue *q, u32 flag)
> +{
> + u32 clr = 1;
> +
> + if (flag & RCB_INT_FLAG_TX)
> + dsaf_write_dev(q, RCBV2_TX_RING_INT_STS_REG, clr);
> +
> + if (flag & RCB_INT_FLAG_RX)
> + dsaf_write_dev(q, RCBV2_RX_RING_INT_STS_REG, clr);
> +}
> +

Why do you need the int_mask_en, clr variables? Why not directly use values?

> +static void fill_v2_desc(struct hnae_ring *ring, void *priv,

> + hnae_set_field(bn_pid, 0x7, 0, buf_num - 1);

Magic values?

> +int hns_nic_net_xmit_hw(struct net_device *ndev,
> + struct sk_buff *skb,
> + struct hns_nic_ring_data *ring_data)
> +{

> - /* If everything has gone correctly network should be the
> + /**
> +  * If everything has gone correctly network should be the
>* data section of the packet and will be the end of the header.
>* If not then it probably represents the end of the last recognized
>* header.

What happened to the network style comments?

>  static int hns_nic_poll_rx_skb(struct hns_nic_ring_data *ring_data,
>  struct sk_buff **out_skb, int *out_bnum)
> + /**
> +  * we will be copying header into skb->data in
> +  * pskb_may_pull so it is in our interest to prefetch
> +  * it now to avoid a possible cache miss
> +  */
> + prefetchw(skb->data);
> +

Likewise

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH V3 net-next 3/5] net:hns: Add Hip06 "TSO(TCP Segment Offload)" support HNS Driver

[Yuval Mintz]

> +static void hns_ae_set_tso_stats(struct hnae_handle *handle, int
> +enable) {
> + struct hns_ppe_cb *ppe_cb = hns_get_ppe_cb(handle);
> +
> + hns_ppe_set_tso_enable(ppe_cb, enable); }

Style issues?

> +void hns_ppe_set_tso_enable(struct hns_ppe_cb *ppe_cb, u32 value) {
> + dsaf_set_dev_bit(ppe_cb, PPEV2_CFG_TSO_EN_REG, 0, !!value); }
> +

Likewise
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH V3 net-next 3/5] net:hns: Add Hip06 "TSO(TCP Segment Offload)" support HNS Driver

[Yuval Mintz]

> +static void hns_ae_set_tso_stats(struct hnae_handle *handle, int
> +enable) {
> + struct hns_ppe_cb *ppe_cb = hns_get_ppe_cb(handle);
> +
> + hns_ppe_set_tso_enable(ppe_cb, enable); }

Style issues?

> +void hns_ppe_set_tso_enable(struct hns_ppe_cb *ppe_cb, u32 value) {
> + dsaf_set_dev_bit(ppe_cb, PPEV2_CFG_TSO_EN_REG, 0, !!value); }
> +

Likewise
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH V4 net-next 4/5] net:hns: Add support of ethtool TSO set option for Hip06 in HNS

[Yuval Mintz]

> +static netdev_features_t hns_nic_fix_features(
> + struct net_device *netdev, netdev_features_t features) {
> + struct hns_nic_priv *priv = netdev_priv(netdev);
> +
> + switch (priv->enet_ver) {
> + case AE_VERSION_1:
> + features &= ~(NETIF_F_TSO | NETIF_F_TSO6 |
> + NETIF_F_HW_VLAN_CTAG_FILTER);
> + break;
> + default:
> + break;
> + }
> + return features;
> +}
> +

Isn't AE_VERSION_1 something fixed once you publish your features?
If it can't be changed, why not simply remove the features from
`hw_features' instead of having to implement this ndo?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH V3 net-next 1/5] net:hns: Add support of Hip06 SoC to the Hislicon Network Subsystem

[Yuval Mintz]

> +void hns_rcbv2_int_ctrl_hw(struct hnae_queue *q, u32 flag, u32 mask)
> +{
> + u32 int_mask_en = !!mask;
> +
> + if (flag & RCB_INT_FLAG_TX)
> + dsaf_write_dev(q, RCB_RING_INTMSK_TXWL_REG,
> int_mask_en);
> +
> + if (flag & RCB_INT_FLAG_RX)
> + dsaf_write_dev(q, RCB_RING_INTMSK_RXWL_REG,
> int_mask_en);
> +}
> +
> +void hns_rcbv2_int_clr_hw(struct hnae_queue *q, u32 flag)
> +{
> + u32 clr = 1;
> +
> + if (flag & RCB_INT_FLAG_TX)
> + dsaf_write_dev(q, RCBV2_TX_RING_INT_STS_REG, clr);
> +
> + if (flag & RCB_INT_FLAG_RX)
> + dsaf_write_dev(q, RCBV2_RX_RING_INT_STS_REG, clr);
> +}
> +

Why do you need the int_mask_en, clr variables? Why not directly use values?

> +static void fill_v2_desc(struct hnae_ring *ring, void *priv,

> + hnae_set_field(bn_pid, 0x7, 0, buf_num - 1);

Magic values?

> +int hns_nic_net_xmit_hw(struct net_device *ndev,
> + struct sk_buff *skb,
> + struct hns_nic_ring_data *ring_data)
> +{

> - /* If everything has gone correctly network should be the
> + /**
> +  * If everything has gone correctly network should be the
>* data section of the packet and will be the end of the header.
>* If not then it probably represents the end of the last recognized
>* header.

What happened to the network style comments?

>  static int hns_nic_poll_rx_skb(struct hns_nic_ring_data *ring_data,
>  struct sk_buff **out_skb, int *out_bnum)
> + /**
> +  * we will be copying header into skb->data in
> +  * pskb_may_pull so it is in our interest to prefetch
> +  * it now to avoid a possible cache miss
> +  */
> + prefetchw(skb->data);
> +

Likewise

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH V3 net-next 2/5] net:hns: Add Hip06 "RSS(Receive Side Scaling)" support to HNS Driver

[Yuval Mintz]

>  static void hns_ppe_init_hw(struct hns_ppe_cb *ppe_cb)  {
...
> + /* Set default RSS key and indrection table*/
> + const u32 rss_key[HNS_PPEV2_RSS_KEY_NUM] = {
> + 0x6d5a56da, 0x255b0ec2,
> + 0x4167253d, 0x43a38fb0,
> + 0xd0ca2bcb, 0xae7b30b4,
> + 0x77cb2da3, 0x8030f20c,
> + 0x6a42b73b, 0xbeac01fa,
> + };
> +
> + /* set default RSS key and remember it */
> + for (i = 0; i < HNS_PPEV2_RSS_KEY_NUM; i++)
> + ppe_cb->rss_key[i]  = rss_key[i];
> 

Is there any reason for the special default key?
Why not use netdev_rss_key_fill()?

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH] bnx2x: use ktime_get_seconds() for timestamp

[Yuval Mintz]

> I assume that it is not possible to change the ABI any more, otherwise
> we should try to use a 64-bit field.

Actually, I did suggest exactly that to our management team,
But this was the decided ABI. 

Acked-by: Yuval Mintz 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH] bnx2x: use ktime_get_seconds() for timestamp

[Yuval Mintz]

> I assume that it is not possible to change the ABI any more, otherwise
> we should try to use a 64-bit field.

Actually, I did suggest exactly that to our management team,
But this was the decided ABI. 

Acked-by: Yuval Mintz <yuval.mi...@qlogic.com>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH 1/2] bnx2x:Fix error handling in the function bnxc2x_dbcx_set_params

[Yuval Mintz]

> I am used to sending out one patch per each fix like this for each function.
> If you don't like me doing this I don't when sending patches for your
> subsystem(s).

I think that would be preferable.

> In addition would something like this fix your complains about not fixing 
> error
> handling in bnx2x_dcbnl_update_applist.
> int bnx2x_dcbnl_update_applist(struct bnx2x *bp, bool delall) {
>   int i, err = 0;
> 
>   for (i = 0; i < DCBX_MAX_APP_PROTOCOL && err == 0; i++) {
...
>   err = dcb_setapp(bp->dev, );
> + if (err)
> + break;

Well, either that or retain the fact there's an error and return it at the end.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH 1/2] bnx2x:Fix error handling in the function bnxc2x_dbcx_set_params

[Yuval Mintz]

> This fixes the error handling in the function bnxc2x_dbcx_params for both 
> calls
> to bnx2x_dcbnl_update_applist by checking if they failed by returning a error
> code and if so return immediately to this function's caller due to this
> nonrecoverable internal failure.

Hi Nicholas,

Even assuming this is the reasonable thing to do [which I'm not fully 
convinced],
I don't think this solution is complete - you'd also need to break the 
iteration in
bnx2x_dcbnl_update_applist() [at the moment said function always returns 'rc'
from the last dcb_app weve tried setting].

Also, why do you send multiple patches for fixing the same issue in different
functions in the same file?

Thanks,
Yuval

> 
> Signed-off-by: Nicholas Krause 
> ---
>  drivers/net/ethernet/broadcom/bnx2x/bnx2x_dcb.c | 6 --
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_dcb.c
> b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_dcb.c
> index 6e4294e..e3cd0c6 100644
> --- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_dcb.c
> +++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_dcb.c
> @@ -724,7 +724,8 @@ void bnx2x_dcbx_set_params(struct bnx2x *bp, u32
> state)
>* Delete app tlvs from dcbnl before reading new
>* negotiation results
>*/
> - bnx2x_dcbnl_update_applist(bp, true);
> + if (bnx2x_dcbnl_update_applist(bp, true))
> + return;
> 
>   /* Read remote mib if dcbx is in the FW */
>   if (bnx2x_dcbx_read_shmem_remote_mib(bp))
> @@ -748,7 +749,8 @@ void bnx2x_dcbx_set_params(struct bnx2x *bp, u32
> state)
>   /*
>* Add new app tlvs to dcbnl
>*/
> - bnx2x_dcbnl_update_applist(bp, false);
> + if (bnx2x_dcbnl_update_applist(bp, false))
> + return;
>  #endif
>   /*
>* reconfigure the netdevice with the results of the new
> --
> 2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH 1/2] bnx2x:Fix error handling in the function bnxc2x_dbcx_set_params

[Yuval Mintz]

 This fixes the error handling in the function bnxc2x_dbcx_params for both 
 calls
 to bnx2x_dcbnl_update_applist by checking if they failed by returning a error
 code and if so return immediately to this function's caller due to this
 nonrecoverable internal failure.

Hi Nicholas,

Even assuming this is the reasonable thing to do [which I'm not fully 
convinced],
I don't think this solution is complete - you'd also need to break the 
iteration in
bnx2x_dcbnl_update_applist() [at the moment said function always returns 'rc'
from the last dcb_app weve tried setting].

Also, why do you send multiple patches for fixing the same issue in different
functions in the same file?

Thanks,
Yuval

 
 Signed-off-by: Nicholas Krause xerofo...@gmail.com
 ---
  drivers/net/ethernet/broadcom/bnx2x/bnx2x_dcb.c | 6 --
  1 file changed, 4 insertions(+), 2 deletions(-)
 
 diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_dcb.c
 b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_dcb.c
 index 6e4294e..e3cd0c6 100644
 --- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_dcb.c
 +++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_dcb.c
 @@ -724,7 +724,8 @@ void bnx2x_dcbx_set_params(struct bnx2x *bp, u32
 state)
* Delete app tlvs from dcbnl before reading new
* negotiation results
*/
 - bnx2x_dcbnl_update_applist(bp, true);
 + if (bnx2x_dcbnl_update_applist(bp, true))
 + return;
 
   /* Read remote mib if dcbx is in the FW */
   if (bnx2x_dcbx_read_shmem_remote_mib(bp))
 @@ -748,7 +749,8 @@ void bnx2x_dcbx_set_params(struct bnx2x *bp, u32
 state)
   /*
* Add new app tlvs to dcbnl
*/
 - bnx2x_dcbnl_update_applist(bp, false);
 + if (bnx2x_dcbnl_update_applist(bp, false))
 + return;
  #endif
   /*
* reconfigure the netdevice with the results of the new
 --
 2.1.4

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH 1/2] bnx2x:Fix error handling in the function bnxc2x_dbcx_set_params

[Yuval Mintz]

 I am used to sending out one patch per each fix like this for each function.
 If you don't like me doing this I don't when sending patches for your
 subsystem(s).

I think that would be preferable.

 In addition would something like this fix your complains about not fixing 
 error
 handling in bnx2x_dcbnl_update_applist.
 int bnx2x_dcbnl_update_applist(struct bnx2x *bp, bool delall) {
   int i, err = 0;
 
   for (i = 0; i  DCBX_MAX_APP_PROTOCOL  err == 0; i++) {
...
   err = dcb_setapp(bp-dev, app);
 + if (err)
 + break;

Well, either that or retain the fact there's an error and return it at the end.

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [bnx2x] Re: Kernel 3.18.11 hangs when inserting netconsle module on a DELL M620 VRTX Blade

[Yuval Mintz]

> > I'have installed a new DELL VRTX M620 Blade with kernel 3.18.11.
> > After system startup I tried to activate the kernel netconsole with remote
> logging enabled.
> >
> > I executed the following command and the shell I issued it becomes
> unresponsive and hangs.
> >
> > #  modprobe netconsole
> netconsole="@/eth0,514@10.1.10.197/00:10:db:fc:60:0c"
> >
> > The system load increases slowly and the CPU #11 uses 100% of soft
> > irq. Only a soft reset witohut loading the netconsole module after startup
> solves the issue.

I suspect this is a regression introduced by 9a2620c87745
"bnx2x: prevent WARN during driver unload".

bnx2x locks & unlocks spin_lock_bh() during the napi poll, which shouldn't
be done while interrupts are disabled. This break interoperability with netpoll,
as it disables irqs prior to sending the skb on the bnx2x's interface.

Can you please try compiling your kernel without CONFIG_NET_RX_BUSY_POLL?
I suspect that might solve your issue.

Regardless, we'll investigate this further and hopefully come up with a fix 
soon.

Thanks,
Yuval

RE: [bnx2x] Re: Kernel 3.18.11 hangs when inserting netconsle module on a DELL M620 VRTX Blade

[Yuval Mintz]

  I'have installed a new DELL VRTX M620 Blade with kernel 3.18.11.
  After system startup I tried to activate the kernel netconsole with remote
 logging enabled.
 
  I executed the following command and the shell I issued it becomes
 unresponsive and hangs.
 
  #  modprobe netconsole
 netconsole=@/eth0,514@10.1.10.197/00:10:db:fc:60:0c
 
  The system load increases slowly and the CPU #11 uses 100% of soft
  irq. Only a soft reset witohut loading the netconsole module after startup
 solves the issue.

I suspect this is a regression introduced by 9a2620c87745
bnx2x: prevent WARN during driver unload.

bnx2x locks  unlocks spin_lock_bh() during the napi poll, which shouldn't
be done while interrupts are disabled. This break interoperability with netpoll,
as it disables irqs prior to sending the skb on the bnx2x's interface.

Can you please try compiling your kernel without CONFIG_NET_RX_BUSY_POLL?
I suspect that might solve your issue.

Regardless, we'll investigate this further and hopefully come up with a fix 
soon.

Thanks,
Yuval

RE: randconfig build error with next-20140825, in drivers/net/ethernet/broadcom/bnx2x

[Yuval Mintz]

> > Building with the attached random configuration file,
> >
> > drivers/built-in.o: In function `__bnx2x_remove':
> > /home/jim/linux/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c:13409:
> > undefined reference to `ptp_clock_unregister'
> > drivers/built-in.o: In function `bnx2x_register_phc':
> > /home/jim/linux/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c:13202:
> > undefined reference to `ptp_clock_register'
> > drivers/built-in.o: In function `bnx2x_get_ts_info':
> >
> /home/jim/linux/drivers/net/ethernet/broadcom/bnx2x/bnx2x_ethtool.c:3498:
> > undefined reference to `ptp_clock_index'
> > make: *** [vmlinux] Error 1
> 
> Hi,I tried to build next-20140825 with your configuration file , but I didn't 
> get any
> error.
> BUILD   arch/x86/boot/bzImage
> Setup is 17068 bytes (padded to 17408 bytes).
> System is 4946 kB
> CRC 13f0e24
> Kernel: arch/x86/boot/bzImage is ready  (#2)
> 
> Am I missing something in the build ?
> 
> Thanks
> Sudip

I believe this is already fixed by commit f79918afb714
("bnx2x: fix build error with ptp") that was submitted a couple of hours ago.

Cheers,
Yuval
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: randconfig build error with next-20140825, in drivers/net/ethernet/broadcom/bnx2x

[Yuval Mintz]

  Building with the attached random configuration file,
 
  drivers/built-in.o: In function `__bnx2x_remove':
  /home/jim/linux/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c:13409:
  undefined reference to `ptp_clock_unregister'
  drivers/built-in.o: In function `bnx2x_register_phc':
  /home/jim/linux/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c:13202:
  undefined reference to `ptp_clock_register'
  drivers/built-in.o: In function `bnx2x_get_ts_info':
 
 /home/jim/linux/drivers/net/ethernet/broadcom/bnx2x/bnx2x_ethtool.c:3498:
  undefined reference to `ptp_clock_index'
  make: *** [vmlinux] Error 1
 
 Hi,I tried to build next-20140825 with your configuration file , but I didn't 
 get any
 error.
 BUILD   arch/x86/boot/bzImage
 Setup is 17068 bytes (padded to 17408 bytes).
 System is 4946 kB
 CRC 13f0e24
 Kernel: arch/x86/boot/bzImage is ready  (#2)
 
 Am I missing something in the build ?
 
 Thanks
 Sudip

I believe this is already fixed by commit f79918afb714
(bnx2x: fix build error with ptp) that was submitted a couple of hours ago.

Cheers,
Yuval
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH 1/3] driver core: enable drivers to use deferred probe from init

[Yuval Mintz]

> On Mon, Jul 28, 2014 at 06:52:48PM +0200, Luis R. Rodriguez wrote:
> > On Mon, Jul 28, 2014 at 03:46:32PM +0000, Yuval Mintz wrote:
> > > Sorry for not being clear, but I didn't meant 'what guarantees that the 
> > > device
> > > will be added to the deferred probe', but rather what guarantees that the
> > > deferred workqueue will be scheduled.
> > >
> > > To the best of my knowledge the deferring mechanism works only if one 
> > > device
> > > is dependent upon another, e.g., for Multi-function devices where one 
> > > device
> > > probe is dependent upon the others - which are soon-to-be probed.
> >
> > The workqueue will be kicked when driver_deferred_probe_trigger() gets
> > poked, we do that in the late_initcall(deferred_probe_initcall), it
> > also gets flushed there with a flush_workqueue(deferred_wq).

> But come to think of it that will work well for devices already plugged in
> so indeed I think that driver_deferred_probe_add() needs a check added
> for for if (!driver_deferred_probe_enable) then we have to
> driver_deferred_probe_trigger(). The driver_deferred_probe_enable is set
> to false upon init, but later on late init it gets set to true so with
> that check we'd  only generate the trigger after late init call.

> I can fold that in the v2.

>   Luis

But what about modules being added after the init-calls? If they try try to use 
this
mechanism, what guarantees they'll eventually get probed?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH 1/3] driver core: enable drivers to use deferred probe from init

[Yuval Mintz]

> Subject: Re: [PATCH 1/3] driver core: enable drivers to use deferred probe 
> from
> init
> 
> On Mon, Jul 28, 2014 at 03:12:11PM +0000, Yuval Mintz wrote:
> > Perhaps this is a silly question, but what guarantees that the
> > deferred probe list will actually be triggered, e.g., in case the
> > delayed device is the last device in the system?
> 
> The dev->init_delayed_probe is used to ensure that we'd add the device to the
> deferred probe list once making this a per device thing if the driver has the 
> field
> delay_probe set to true. This technically also allows this to be a per device 
> thing
> so with some more work we could enable drivers to only enable this for 
> specific
> devices but at this point this did not seem required.

Sorry for not being clear, but I didn't meant 'what guarantees that the device
will be added to the deferred probe', but rather what guarantees that the
deferred workqueue will be scheduled.

To the best of my knowledge the deferring mechanism works only if one device
is dependent upon another, e.g., for Multi-function devices where one device
probe is dependent upon the others - which are soon-to-be probed.

[If I'm incorrect I'd appreciate if someone could correct me]

>
> > [From drivers/base/dd.c  - "A successful driver probe will trigger
> > moving all devices from the pending to the active list so that the
> > workqueue will eventually retry them]
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH 1/3] driver core: enable drivers to use deferred probe from init

[Yuval Mintz]

> +static int __driver_probe_device(struct device_driver *drv, struct
> +device *dev) {
> + if (drv->delay_probe && !dev->init_delayed_probe) {
> + dev_info(dev, "Driver %s requests probe deferral on init\n",
> +  drv->name);
> + dev->init_delayed_probe = true;
> + driver_deferred_probe_add(dev);
> + return -EPROBE_DEFER;
> + }
> +
> + return really_probe(dev, drv);
> +}

Perhaps this is a silly question, but what guarantees that the deferred probe
list will actually be triggered, e.g., in case the delayed device is the last 
device
in the system?

[From drivers/base/dd.c  - "A successful driver probe will trigger moving all
devices from the pending to the active list so that the workqueue will
eventually retry them]

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH 1/3] driver core: enable drivers to use deferred probe from init

[Yuval Mintz]

 +static int __driver_probe_device(struct device_driver *drv, struct
 +device *dev) {
 + if (drv-delay_probe  !dev-init_delayed_probe) {
 + dev_info(dev, Driver %s requests probe deferral on init\n,
 +  drv-name);
 + dev-init_delayed_probe = true;
 + driver_deferred_probe_add(dev);
 + return -EPROBE_DEFER;
 + }
 +
 + return really_probe(dev, drv);
 +}

Perhaps this is a silly question, but what guarantees that the deferred probe
list will actually be triggered, e.g., in case the delayed device is the last 
device
in the system?

[From drivers/base/dd.c  - A successful driver probe will trigger moving all
devices from the pending to the active list so that the workqueue will
eventually retry them]

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH 1/3] driver core: enable drivers to use deferred probe from init

[Yuval Mintz]

 Subject: Re: [PATCH 1/3] driver core: enable drivers to use deferred probe 
 from
 init
 
 On Mon, Jul 28, 2014 at 03:12:11PM +, Yuval Mintz wrote:
  Perhaps this is a silly question, but what guarantees that the
  deferred probe list will actually be triggered, e.g., in case the
  delayed device is the last device in the system?
 
 The dev-init_delayed_probe is used to ensure that we'd add the device to the
 deferred probe list once making this a per device thing if the driver has the 
 field
 delay_probe set to true. This technically also allows this to be a per device 
 thing
 so with some more work we could enable drivers to only enable this for 
 specific
 devices but at this point this did not seem required.

Sorry for not being clear, but I didn't meant 'what guarantees that the device
will be added to the deferred probe', but rather what guarantees that the
deferred workqueue will be scheduled.

To the best of my knowledge the deferring mechanism works only if one device
is dependent upon another, e.g., for Multi-function devices where one device
probe is dependent upon the others - which are soon-to-be probed.

[If I'm incorrect I'd appreciate if someone could correct me]


  [From drivers/base/dd.c  - A successful driver probe will trigger
  moving all devices from the pending to the active list so that the
  workqueue will eventually retry them]
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH 1/3] driver core: enable drivers to use deferred probe from init

[Yuval Mintz]

 On Mon, Jul 28, 2014 at 06:52:48PM +0200, Luis R. Rodriguez wrote:
  On Mon, Jul 28, 2014 at 03:46:32PM +, Yuval Mintz wrote:
   Sorry for not being clear, but I didn't meant 'what guarantees that the 
   device
   will be added to the deferred probe', but rather what guarantees that the
   deferred workqueue will be scheduled.
  
   To the best of my knowledge the deferring mechanism works only if one 
   device
   is dependent upon another, e.g., for Multi-function devices where one 
   device
   probe is dependent upon the others - which are soon-to-be probed.
 
  The workqueue will be kicked when driver_deferred_probe_trigger() gets
  poked, we do that in the late_initcall(deferred_probe_initcall), it
  also gets flushed there with a flush_workqueue(deferred_wq).

 But come to think of it that will work well for devices already plugged in
 so indeed I think that driver_deferred_probe_add() needs a check added
 for for if (!driver_deferred_probe_enable) then we have to
 driver_deferred_probe_trigger(). The driver_deferred_probe_enable is set
 to false upon init, but later on late init it gets set to true so with
 that check we'd  only generate the trigger after late init call.

 I can fold that in the v2.

   Luis

But what about modules being added after the init-calls? If they try try to use 
this
mechanism, what guarantees they'll eventually get probed?
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH v2 2/3] net: davinci_mdio: Convert pr_err() to dev_err() call

[Yuval Mintz]

> Also, Convert kzalloc to devm_kzalloc.

Looks like you should remove this part of the comment.

>
> Signed-off-by: George Cherian 
> Reviewed-by: Felipe Balbi 
> ---
>  drivers/net/ethernet/ti/davinci_mdio.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/ti/davinci_mdio.c
> b/drivers/net/ethernet/ti/davinci_mdio.c
> index 34e97ec..735dc53 100644
> --- a/drivers/net/ethernet/ti/davinci_mdio.c
> +++ b/drivers/net/ethernet/ti/davinci_mdio.c
> @@ -303,7 +303,7 @@ static int davinci_mdio_probe_dt(struct
> mdio_platform_data *data,
>   return -EINVAL;
>
>   if (of_property_read_u32(node, "bus_freq", )) {
> - pr_err("Missing bus_freq property in the DT.\n");
> + dev_err(>dev, "Missing bus_freq property in the DT.\n");
>   return -EINVAL;
>   }
>   data->bus_freq = prop;



This message and any attached documents contain information from QLogic 
Corporation or its wholly-owned subsidiaries that may be confidential. If you 
are not the intended recipient, you may not read, copy, distribute, or use this 
information. If you have received this transmission in error, please notify the 
sender immediately by reply e-mail and then delete this message.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH v2 2/3] net: davinci_mdio: Convert pr_err() to dev_err() call

[Yuval Mintz]

 Also, Convert kzalloc to devm_kzalloc.

Looks like you should remove this part of the comment.


 Signed-off-by: George Cherian george.cher...@ti.com
 Reviewed-by: Felipe Balbi ba...@ti.com
 ---
  drivers/net/ethernet/ti/davinci_mdio.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

 diff --git a/drivers/net/ethernet/ti/davinci_mdio.c
 b/drivers/net/ethernet/ti/davinci_mdio.c
 index 34e97ec..735dc53 100644
 --- a/drivers/net/ethernet/ti/davinci_mdio.c
 +++ b/drivers/net/ethernet/ti/davinci_mdio.c
 @@ -303,7 +303,7 @@ static int davinci_mdio_probe_dt(struct
 mdio_platform_data *data,
   return -EINVAL;

   if (of_property_read_u32(node, bus_freq, prop)) {
 - pr_err(Missing bus_freq property in the DT.\n);
 + dev_err(pdev-dev, Missing bus_freq property in the DT.\n);
   return -EINVAL;
   }
   data-bus_freq = prop;



This message and any attached documents contain information from QLogic 
Corporation or its wholly-owned subsidiaries that may be confidential. If you 
are not the intended recipient, you may not read, copy, distribute, or use this 
information. If you have received this transmission in error, please notify the 
sender immediately by reply e-mail and then delete this message.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: bnx2x_sriov.c: Missing switch/case breaks?

[Yuval Mintz]

> > > Hi Ariel.
> > >
> > > I wrote a little checkpatch script to look for missing
> > > switch/case breaks.
> > >
> > > http://www.kernelhub.org/?msg=379933=2
> > >
> > > There are _many_ instances of case blocks in sriov.c
> > > that could be missing breaks as they use fall-throughs.
> > >
> > > It would be good if these are actually intended to be
> > > fall-throughs to add a /* fall-through */ comment between
> > > each case block.
> > >
> > Hi Joe,
> 
> Hi Yuval.
> 
> > The `vfop' part of the code contains a lot of usage of the
> `bnx2x_vfop_finalize()',
> > which either goto or return at the end of almost every case.
> > "Normal" analysis tools/scripts fail to recognize them as  valid case
> breaks.
> >
> > Adding `fallthrough' comments would make little sense, as this is not the
> real
> > behavior; Perhaps we need some alternative comment? (something in the
> line
> > of `macro case break')
> 
> No idea.  It's certainly an ugly macro.
> 

True.

> This does have a fallthrough path though when
> (rc == 0  && next == VFOP_VERIFY_PEND) so

This is a very rare path - there's exactly one place in the bnx2x code
Where `next == VFOP_VERIFY_PEND' (also notice this path prints an
error, so this is obviously not the expected behaviour).

> maybe there should be a break after most all
> uses of this macro anyway.  When next is

Won't some static code analysis tools regard such `break' calls as
unreachable code?

> VFOP_VERIFY_PEND, then a "fall-through" comment
> would be appropriate.
> 
> cheers, Joe

Thanks,
Yuval
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: bnx2x_sriov.c: Missing switch/case breaks?

[Yuval Mintz]

   Hi Ariel.
  
   I wrote a little checkpatch script to look for missing
   switch/case breaks.
  
   http://www.kernelhub.org/?msg=379933p=2
  
   There are _many_ instances of case blocks in sriov.c
   that could be missing breaks as they use fall-throughs.
  
   It would be good if these are actually intended to be
   fall-throughs to add a /* fall-through */ comment between
   each case block.
  
  Hi Joe,
 
 Hi Yuval.
 
  The `vfop' part of the code contains a lot of usage of the
 `bnx2x_vfop_finalize()',
  which either goto or return at the end of almost every case.
  Normal analysis tools/scripts fail to recognize them as  valid case
 breaks.
 
  Adding `fallthrough' comments would make little sense, as this is not the
 real
  behavior; Perhaps we need some alternative comment? (something in the
 line
  of `macro case break')
 
 No idea.  It's certainly an ugly macro.
 

True.

 This does have a fallthrough path though when
 (rc == 0   next == VFOP_VERIFY_PEND) so

This is a very rare path - there's exactly one place in the bnx2x code
Where `next == VFOP_VERIFY_PEND' (also notice this path prints an
error, so this is obviously not the expected behaviour).

 maybe there should be a break after most all
 uses of this macro anyway.  When next is

Won't some static code analysis tools regard such `break' calls as
unreachable code?

 VFOP_VERIFY_PEND, then a fall-through comment
 would be appropriate.
 
 cheers, Joe

Thanks,
Yuval
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: bnx2x_sriov.c: Missing switch/case breaks?

[Yuval Mintz]

> Hi Ariel.
> 
> I wrote a little checkpatch script to look for missing
> switch/case breaks.
> 
> http://www.kernelhub.org/?msg=379933=2
> 
> There are _many_ instances of case blocks in sriov.c
> that could be missing breaks as they use fall-throughs.
> 
> It would be good if these are actually intended to be
> fall-throughs to add a /* fall-through */ comment between
> each case block.
> 
> For instance:
> 
> static void bnx2x_vfop_qctor(struct bnx2x *bp, struct bnx2x_virtf *vf)
> {
> [...]
>   switch (state) {
>   case BNX2X_VFOP_QCTOR_INIT:
> 
>   /* has this queue already been opened? */
>   if (bnx2x_get_q_logical_state(bp, q_params->q_obj) ==
>   BNX2X_Q_LOGICAL_STATE_ACTIVE) {
>   DP(BNX2X_MSG_IOV,
>  "Entered qctor but queue was already up. Aborting
> gracefully\n");
>   goto op_done;
>   }
> 
>   /* next state */
>   vfop->state = BNX2X_VFOP_QCTOR_SETUP;
> 
>   q_params->cmd = BNX2X_Q_CMD_INIT;
>   vfop->rc = bnx2x_queue_state_change(bp, q_params);
> 
>   bnx2x_vfop_finalize(vf, vfop->rc, VFOP_CONT); 

Hi Joe,

The `vfop' part of the code contains a lot of usage of the 
`bnx2x_vfop_finalize()',
which either goto or return at the end of almost every case.
"Normal" analysis tools/scripts fail to recognize them as  valid case breaks.

Adding `fallthrough' comments would make little sense, as this is not the real
behavior; Perhaps we need some alternative comment? (something in the line
of `macro case break')

Cheers,
Yuval

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: bnx2x_sriov.c: Missing switch/case breaks?

[Yuval Mintz]

 Hi Ariel.
 
 I wrote a little checkpatch script to look for missing
 switch/case breaks.
 
 http://www.kernelhub.org/?msg=379933p=2
 
 There are _many_ instances of case blocks in sriov.c
 that could be missing breaks as they use fall-throughs.
 
 It would be good if these are actually intended to be
 fall-throughs to add a /* fall-through */ comment between
 each case block.
 
 For instance:
 
 static void bnx2x_vfop_qctor(struct bnx2x *bp, struct bnx2x_virtf *vf)
 {
 [...]
   switch (state) {
   case BNX2X_VFOP_QCTOR_INIT:
 
   /* has this queue already been opened? */
   if (bnx2x_get_q_logical_state(bp, q_params-q_obj) ==
   BNX2X_Q_LOGICAL_STATE_ACTIVE) {
   DP(BNX2X_MSG_IOV,
  Entered qctor but queue was already up. Aborting
 gracefully\n);
   goto op_done;
   }
 
   /* next state */
   vfop-state = BNX2X_VFOP_QCTOR_SETUP;
 
   q_params-cmd = BNX2X_Q_CMD_INIT;
   vfop-rc = bnx2x_queue_state_change(bp, q_params);
 
   bnx2x_vfop_finalize(vf, vfop-rc, VFOP_CONT); 

Hi Joe,

The `vfop' part of the code contains a lot of usage of the 
`bnx2x_vfop_finalize()',
which either goto or return at the end of almost every case.
Normal analysis tools/scripts fail to recognize them as  valid case breaks.

Adding `fallthrough' comments would make little sense, as this is not the real
behavior; Perhaps we need some alternative comment? (something in the line
of `macro case break')

Cheers,
Yuval

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH] net-bnx2x: Fix byte order problem on NVRAM writes

[Yuval Mintz]

> Tested:
> ethtool -e eth0 raw on >first.nvram
> ethtool -E eth0  ethtool -e eth0 raw on >second.nvram
> cmp first.nvram second.nvram || ethtool -E eth0  (No output means pass.)

Hi Nate,

We're aware of this `bug' for some time - we've encountered it when
trying to fix the endian sparse warnings in the driver.

Sadly, there are already existing user applications that assume that this is 
the driver's behaviour - i.e., those applications prepare their buffers in a 
manner which assumes the endian of the writes; changing this write will 
cause those tools to break.
That's why we haven't fixed the issue before, and cannot support such a
fix. We're more than willing to document it somewhere, if that seems 
useful to anyone.

Thanks,
Yuval

> ---
>  drivers/net/ethernet/broadcom/bnx2x/bnx2x_ethtool.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_ethtool.c
> b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_ethtool.c
> index 8213cc8..35671fb 100644
> --- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_ethtool.c
> +++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_ethtool.c
> @@ -1549,7 +1549,7 @@ static int bnx2x_nvram_write_dword(struct bnx2x
> *bp, u32 offset, u32 val,
>   REG_WR(bp, MCP_REG_MCPR_NVM_COMMAND,
> MCPR_NVM_COMMAND_DONE);
> 
>   /* write the data */
> - REG_WR(bp, MCP_REG_MCPR_NVM_WRITE, val);
> + REG_WR(bp, MCP_REG_MCPR_NVM_WRITE, cpu_to_be32(val));
> 
>   /* address of the NVRAM to write to */
>   REG_WR(bp, MCP_REG_MCPR_NVM_ADDR,
> --


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH] net-bnx2x: Fix byte order problem on NVRAM writes

[Yuval Mintz]

 Tested:
 ethtool -e eth0 raw on first.nvram
 ethtool -E eth0 first.nvram
 ethtool -e eth0 raw on second.nvram
 cmp first.nvram second.nvram || ethtool -E eth0 second.nvram
 (No output means pass.)

Hi Nate,

We're aware of this `bug' for some time - we've encountered it when
trying to fix the endian sparse warnings in the driver.

Sadly, there are already existing user applications that assume that this is 
the driver's behaviour - i.e., those applications prepare their buffers in a 
manner which assumes the endian of the writes; changing this write will 
cause those tools to break.
That's why we haven't fixed the issue before, and cannot support such a
fix. We're more than willing to document it somewhere, if that seems 
useful to anyone.

Thanks,
Yuval

 ---
  drivers/net/ethernet/broadcom/bnx2x/bnx2x_ethtool.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
 
 diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_ethtool.c
 b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_ethtool.c
 index 8213cc8..35671fb 100644
 --- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_ethtool.c
 +++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_ethtool.c
 @@ -1549,7 +1549,7 @@ static int bnx2x_nvram_write_dword(struct bnx2x
 *bp, u32 offset, u32 val,
   REG_WR(bp, MCP_REG_MCPR_NVM_COMMAND,
 MCPR_NVM_COMMAND_DONE);
 
   /* write the data */
 - REG_WR(bp, MCP_REG_MCPR_NVM_WRITE, val);
 + REG_WR(bp, MCP_REG_MCPR_NVM_WRITE, cpu_to_be32(val));
 
   /* address of the NVRAM to write to */
   REG_WR(bp, MCP_REG_MCPR_NVM_ADDR,
 --


--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH 1/9] Bnx2x: remove redundant D0 power state set

[Yuval Mintz]

> Pci_enable_device() will set device power state to D0,
> so it's no need to do it again in bnx2x_init_dev().
> Also remove redundant PM Cap find code, because pci core
> has been saved the pci device pm cap value.
> 
> Signed-off-by: Yijing Wang 
> Cc: Eilon Greenstein 
> Cc: net...@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> ---
[...]

Acked-by: Yuval Mintz 

Thanks,
Yuval

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH 1/9] Bnx2x: remove redundant D0 power state set

[Yuval Mintz]

 Pci_enable_device() will set device power state to D0,
 so it's no need to do it again in bnx2x_init_dev().
 Also remove redundant PM Cap find code, because pci core
 has been saved the pci device pm cap value.
 
 Signed-off-by: Yijing Wang wangyij...@huawei.com
 Cc: Eilon Greenstein eil...@broadcom.com
 Cc: net...@vger.kernel.org
 Cc: linux-kernel@vger.kernel.org
 ---
[...]

Acked-by: Yuval Mintz yuval...@broadcom.com

Thanks,
Yuval

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: linux-next: manual merge of the net-next tree with the net tree

[Yuval Mintz]

> Hi all,
> 
> Today's linux-next merge of the net-next tree got a conflict in
> drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c between commit
> ecf01c22be03 ("bnx2x: Prevent NULL pointer dereference in kdump") from
> the net tree and commit 5b0752c863d7 ("bnx2x: Fix VF statistics") from
> the net-next tree.
> 
> I am not sure about this, but I just used the net tree version but I
> assume something more may be needed and can carry the fix as
> necessary (no action is required).
> 

Hi Stephen,

This patch adds the missing functionality from the `net-next' patch
which was lost due to the merge.

Is `linux-next' the correct tree to send it into?
(I couldn't designate it to `net-next' as the merge between `net' and
`net-next' hasn't occured yet, thus no collision there)

Signed-off-by: Yuval Mintz 
Signed-off-by: Ariel Elior 
---
 drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c index
06dee5c..52cf71c 100644 ---
a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c +++
b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c @@ -6053,6 +6053,8
@@ void bnx2x_pre_irq_nic_init(struct bnx2x *bp) bnx2x_init_def_sb(bp);
bnx2x_update_dsb_idx(bp);
bnx2x_init_sp_ring(bp);
+   } else {
+   bnx2x_memset_stats(bp);
}
 }
 
-- 
1.8.1.227.g44fe835

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: linux-next: manual merge of the net-next tree with the net tree

[Yuval Mintz]

 Hi all,
 
 Today's linux-next merge of the net-next tree got a conflict in
 drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c between commit
 ecf01c22be03 (bnx2x: Prevent NULL pointer dereference in kdump) from
 the net tree and commit 5b0752c863d7 (bnx2x: Fix VF statistics) from
 the net-next tree.
 
 I am not sure about this, but I just used the net tree version but I
 assume something more may be needed and can carry the fix as
 necessary (no action is required).
 

Hi Stephen,

This patch adds the missing functionality from the `net-next' patch
which was lost due to the merge.

Is `linux-next' the correct tree to send it into?
(I couldn't designate it to `net-next' as the merge between `net' and
`net-next' hasn't occured yet, thus no collision there)

Signed-off-by: Yuval Mintz yuval...@broadcom.com
Signed-off-by: Ariel Elior ari...@broadcom.com
---
 drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c index
06dee5c..52cf71c 100644 ---
a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c +++
b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c @@ -6053,6 +6053,8
@@ void bnx2x_pre_irq_nic_init(struct bnx2x *bp) bnx2x_init_def_sb(bp);
bnx2x_update_dsb_idx(bp);
bnx2x_init_sp_ring(bp);
+   } else {
+   bnx2x_memset_stats(bp);
}
 }
 
-- 
1.8.1.227.g44fe835

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: linux-next: manual merge of the pci tree with Linus' tree

[Yuval Mintz]

> Today's linux-next merge of the pci tree got a conflict in
> drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c between commit
> 8eee694c3e66 ("bnx2x: fix unload previous driver flow when flr-capable")
> from Linus' tree and commit 2a80eebcbf3e ("bnx2x: Use PCI Express
> Capability accessors") from the pci tree.
> 
> The former removes the function updated by the latter, so I just removed
> the function (see below) and can carry the fix as necessary.

Acked-by: Yuval Mintz 


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: linux-next: manual merge of the pci tree with Linus' tree

[Yuval Mintz]

 Today's linux-next merge of the pci tree got a conflict in
 drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c between commit
 8eee694c3e66 (bnx2x: fix unload previous driver flow when flr-capable)
 from Linus' tree and commit 2a80eebcbf3e (bnx2x: Use PCI Express
 Capability accessors) from the pci tree.
 
 The former removes the function updated by the latter, so I just removed
 the function (see below) and can carry the fix as necessary.

Acked-by: Yuval Mintz yuval...@broadcom.com


--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/