Re: [PATCH net v2] i40e: fix the panic when running bpf in xdpdrv mode

[Jesse Brandeburg]

Jason Xing wrote:

> On Wed, Apr 14, 2021 at 12:27 AM Jesse Brandeburg
>  wrote:
> >
> > kerneljasonx...@gmail.com wrote:
> >
> > > From: Jason Xing 
> >
> > Hi Jason,
> >
> > Sorry, I missed this on the first time: Added intel-wired-lan,
> > please include on any future submissions for Intel drivers.
> > get-maintainers script might help here?
> >
> 
> Probably I got this wrong in the last email. Did you mean that I should add
> intel-wired-lan in the title not the cc list? It seems I should put
> this together on
> the next submission like this:
> 
> [Intel-wired-lan] [PATCH net v4]

Your v3 submittal was correct. My intent was to make sure
intel-wired-lan was in CC:

If Kuba or Dave wants us to take the fix in via intel-wired-lan trees,
then we can do that, or they can apply it directly. I'll ack it on the
v3.

Re: [PATCH net v3] i40e: fix the panic when running bpf in xdpdrv mode

[Jesse Brandeburg]

kerneljasonx...@gmail.com wrote:

> From: Jason Xing 
> 
> Fix this panic by adding more rules to calculate the value of @rss_size_max
> which could be used in allocating the queues when bpf is loaded, which,
> however, could cause the failure and then trigger the NULL pointer of
> vsi->rx_rings. Prio to this fix, the machine doesn't care about how many
> cpus are online and then allocates 256 queues on the machine with 32 cpus
> online actually.
> 
> Once the load of bpf begins, the log will go like this "failed to get
> tracking for 256 queues for VSI 0 err -12" and this "setup of MAIN VSI
> failed".
> 
> Thus, I attach the key information of the crash-log here.
> 
> BUG: unable to handle kernel NULL pointer dereference at
> 
> RIP: 0010:i40e_xdp+0xdd/0x1b0 [i40e]
> Call Trace:
> [2160294.717292]  ? i40e_reconfig_rss_queues+0x170/0x170 [i40e]
> [2160294.717666]  dev_xdp_install+0x4f/0x70
> [2160294.718036]  dev_change_xdp_fd+0x11f/0x230
> [2160294.718380]  ? dev_disable_lro+0xe0/0xe0
> [2160294.718705]  do_setlink+0xac7/0xe70
> [2160294.719035]  ? __nla_parse+0xed/0x120
> [2160294.719365]  rtnl_newlink+0x73b/0x860
> 
> Fixes: 41c445ff0f48 ("i40e: main driver core")
> Co-developed-by: Shujin Li 
> Signed-off-by: Shujin Li 
> Signed-off-by: Jason Xing 

Reviewed-by: Jesse Brandeburg 

@Jakub/@DaveM - feel free to apply this directly.

Re: [Patch v4 1/3] lib: Restrict cpumask_local_spread to houskeeping CPUs

[Jesse Brandeburg]

Nitesh Narayan Lal wrote:

> > The original issue as seen, was that if you rmmod/insmod a driver
> > *without* irqbalance running, the default irq mask is -1, which means
> > any CPU. The older kernels (this issue was patched in 2014) used to use
> > that affinity mask, but the value programmed into all the interrupt
> > registers "actual affinity" would end up delivering all interrupts to
> > CPU0,
> 
> So does that mean the affinity mask for the IRQs was different wrt where
> the IRQs were actually delivered?
> Or, the affinity mask itself for the IRQs after rmmod, insmod was changed
> to 0 instead of -1?

The smp_affinity was 0xfff, and the kernel chooses which interrupt to
place the interrupt on, among any of the bits set.

 
> I did a quick test on top of 5.12.0-rc6 by comparing the i40e IRQ affinity
> mask before removing the kernel module and after doing rmmod+insmod
> and didn't find any difference.

with the patch in question removed? Sorry, I'm confused what you tried.

> 
> >  and if the machine was under traffic load incoming when the
> > driver loaded, CPU0 would start to poll among all the different netdev
> > queues, all on CPU0.
> >
> > The above then leads to the condition that the device is stuck polling
> > even if the affinity gets updated from user space, and the polling will
> > continue until traffic stops.
> >
> >> The problem with the commit is that when we overwrite the affinity mask
> >> based on the hinting mask we completely ignore the default SMP affinity
> >> mask. If we do want to overwrite the affinity based on the hint mask we
> >> should atleast consider the default SMP affinity.
> 
> For the issue where the IRQs don't follow the default_smp_affinity mask
> because of this patch, the following are the steps by which it can be easily
> reproduced with the latest linux kernel:
> 
> # Kernel
> 5.12.0-rc6+



> As we can see in the above trace the initial affinity for the IRQ 1478 was
> correctly set as per the default_smp_affinity mask which includes CPU 42,
> however, later on, it is updated with CPU3 which is returned from
> cpumask_local_spread().
> 
> > Maybe the right thing is to fix which CPUs are passed in as the valid
> > mask, or make sure the kernel cross checks that what the driver asks
> > for is a "valid CPU"?
> >
> 
> Sure, if we can still reproduce the problem that your patch was fixing then
> maybe we can consider adding a new API like cpumask_local_spread_irq in
> which we should consider deafult_smp_affinity mask as well before returning
> the CPU.

I'm sure I don't have a reproducer of the original problem any more, it
is lost somewhere 8 years ago. I'd like to be able to repro the original
issue, but I can't.

Your description of the problem makes it obvious there is an issue. It
appears as if cpumask_local_spread() is the wrong function to use here.
If you have any suggestions please let me know.

We had one other report of this problem as well (I'm not sure if it's
the same as your report)
https://lkml.org/lkml/2021/3/28/206
https://lists.osuosl.org/pipermail/intel-wired-lan/Week-of-Mon-20210125/023120.html

Re: [PATCH net v2] i40e: fix the panic when running bpf in xdpdrv mode

[Jesse Brandeburg]

kerneljasonx...@gmail.com wrote:

> From: Jason Xing 

Hi Jason,

Sorry, I missed this on the first time: Added intel-wired-lan,
please include on any future submissions for Intel drivers.
get-maintainers script might help here?

> 
> Fix this panic by adding more rules to calculate the value of @rss_size_max
> which could be used in allocating the queues when bpf is loaded, which,
> however, could cause the failure and then trigger the NULL pointer of
> vsi->rx_rings. Prio to this fix, the machine doesn't care about how many
> cpus are online and then allocates 256 queues on the machine with 32 cpus
> online actually.
> 
> Once the load of bpf begins, the log will go like this "failed to get
> tracking for 256 queues for VSI 0 err -12" and this "setup of MAIN VSI
> failed".
> 
> Thus, I attach the key information of the crash-log here.
> 
> BUG: unable to handle kernel NULL pointer dereference at
> 
> RIP: 0010:i40e_xdp+0xdd/0x1b0 [i40e]
> Call Trace:
> [2160294.717292]  ? i40e_reconfig_rss_queues+0x170/0x170 [i40e]
> [2160294.717666]  dev_xdp_install+0x4f/0x70
> [2160294.718036]  dev_change_xdp_fd+0x11f/0x230
> [2160294.718380]  ? dev_disable_lro+0xe0/0xe0
> [2160294.718705]  do_setlink+0xac7/0xe70
> [2160294.719035]  ? __nla_parse+0xed/0x120
> [2160294.719365]  rtnl_newlink+0x73b/0x860
> 
> Fixes: 41c445ff0f48 ("i40e: main driver core")
> 

This Fixes line should be connected to the Sign offs with
no linefeeds between.

> Signed-off-by: Jason Xing 
> Signed-off-by: Shujin Li 

Did Shujin contribute to this patch? Why are they signing off? If
they developed this patch with you, it should say:
Co-developed-by: Shujin 
Signed-off-by: Shujin ...
Signed-off-by: Jason ...

Your signature should be last if you sent the patch. The sign-offs are
like a chain of custody, please review 
https://www.kernel.org/doc/html/latest/process/submitting-patches.html#when-to-use-acked-by-cc-and-co-developed-by

Thanks,
 Jesse

Re: [PATCH] i40e: fix the panic when running bpf in xdpdrv mode

[Jesse Brandeburg]

kerneljasonx...@gmail.com wrote:

> From: Jason Xing 
> 
> Re: [PATCH] i40e: fix the panic when running bpf in xdpdrv mode

Please use netdev style subject lines when patching net kernel to
indicate which kernel tree this is targeted at, "net" or "net-next"
[PATCH net v2] i40e: ...

> Fix this by add more rules to calculate the value of @rss_size_max which

Fix this panic by adding ...

> could be used in allocating the queues when bpf is loaded, which, however,
> could cause the failure and then triger the NULL pointer of vsi->rx_rings.

trigger

> Prio to this fix, the machine doesn't care about how many cpus are online
> and then allocates 256 queues on the machine with 32 cpus online
> actually.
> 
> Once the load of bpf begins, the log will go like this "failed to get
> tracking for 256 queues for VSI 0 err -12" and this "setup of MAIN VSI
> failed".
> 
> Thus, I attach the key information of the crash-log here.
> 
> BUG: unable to handle kernel NULL pointer dereference at
> 
> RIP: 0010:i40e_xdp+0xdd/0x1b0 [i40e]
> Call Trace:
> [2160294.717292]  ? i40e_reconfig_rss_queues+0x170/0x170 [i40e]
> [2160294.717666]  dev_xdp_install+0x4f/0x70
> [2160294.718036]  dev_change_xdp_fd+0x11f/0x230
> [2160294.718380]  ? dev_disable_lro+0xe0/0xe0
> [2160294.718705]  do_setlink+0xac7/0xe70
> [2160294.719035]  ? __nla_parse+0xed/0x120
> [2160294.719365]  rtnl_newlink+0x73b/0x860
> 
> Signed-off-by: Jason Xing 
> Signed-off-by: Shujin Li 

if you send to "net" - I suspect you should supply a Fixes: line, above
the sign-offs.
In this case however, this bug has been here since the beginning of the
driver, but the patch will easily apply, so please supply

Fixes: 41c445ff0f48 ("i40e: main driver core")

> ---
>  drivers/net/ethernet/intel/i40e/i40e_main.c | 6 ++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c 
> b/drivers/net/ethernet/intel/i40e/i40e_main.c
> index 521ea9d..4e9a247 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_main.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
> @@ -11867,6 +11867,7 @@ static int i40e_sw_init(struct i40e_pf *pf)
>  {
>   int err = 0;
>   int size;
> + u16 pow;
>  
>   /* Set default capability flags */
>   pf->flags = I40E_FLAG_RX_CSUM_ENABLED |
> @@ -11885,6 +11886,11 @@ static int i40e_sw_init(struct i40e_pf *pf)
>   pf->rss_table_size = pf->hw.func_caps.rss_table_size;
>   pf->rss_size_max = min_t(int, pf->rss_size_max,
>pf->hw.func_caps.num_tx_qp);
> +
> + /* find the next higher power-of-2 of num cpus */
> + pow = roundup_pow_of_two(num_online_cpus());
> + pf->rss_size_max = min_t(int, pf->rss_size_max, pow);
> +

The fix itself is fine, and is correct as far as I can tell, thank you
for sending the patch!

>   if (pf->hw.func_caps.rss) {
>   pf->flags |= I40E_FLAG_RSS_ENABLED;
>   pf->alloc_rss_size = min_t(int, pf->rss_size_max,

Re: [Patch v4 1/3] lib: Restrict cpumask_local_spread to houskeeping CPUs

[Jesse Brandeburg]

Continuing a thread from a bit ago...

Nitesh Narayan Lal wrote:

> > After a little more digging, I found out why cpumask_local_spread change
> > affects the general/initial smp_affinity for certain device IRQs.
> >
> > After the introduction of the commit:
> >
> >     e2e64a932 genirq: Set initial affinity in irq_set_affinity_hint()
> >
> 
> Continuing the conversation about the above commit and adding Jesse.
> I was trying to understand the problem that the commit message explains
> "The default behavior of the kernel is somewhat undesirable as all
> requested interrupts end up on CPU0 after registration.", I have also been
> trying to reproduce this behavior without the patch but I failed in doing
> so, maybe because I am missing something here.
> 
> @Jesse Can you please explain? FWIU IRQ affinity should be decided based on
> the default affinity mask.

The original issue as seen, was that if you rmmod/insmod a driver
*without* irqbalance running, the default irq mask is -1, which means
any CPU. The older kernels (this issue was patched in 2014) used to use
that affinity mask, but the value programmed into all the interrupt
registers "actual affinity" would end up delivering all interrupts to
CPU0, and if the machine was under traffic load incoming when the
driver loaded, CPU0 would start to poll among all the different netdev
queues, all on CPU0.

The above then leads to the condition that the device is stuck polling
even if the affinity gets updated from user space, and the polling will
continue until traffic stops.

> The problem with the commit is that when we overwrite the affinity mask
> based on the hinting mask we completely ignore the default SMP affinity
> mask. If we do want to overwrite the affinity based on the hint mask we
> should atleast consider the default SMP affinity.

Maybe the right thing is to fix which CPUs are passed in as the valid
mask, or make sure the kernel cross checks that what the driver asks
for is a "valid CPU"?

Re: [PATCH] net: ethernet: intel: Fix a typo in the file ixgbe_dcb_nl.c

[Jesse Brandeburg]

Bhaskar Chowdhury wrote:

> 
> s/Reprogam/Reprogram/
> 
> Signed-off-by: Bhaskar Chowdhury 

Reviewed-by: Jesse Brandeburg 

Re: [page-reclaim] Augmented Page Reclaim

[Jesse Barnes]

> ==
> Augmented Page Reclaim
> ==
> We would like to share a work with you and see if there is enough
> interest to warrant a run for the mainline. This work is a part of
> result from a decade of research and experimentation in memory
> overcommit at Google: an augmented page reclaim that, in our
> experience, is performant, versatile and, more importantly, simple.

Per discussion on IRC, maybe some additional background would help.

In looking at browser workloads on Chrome OS, we found that reclaim was:
1) too expensive in terms of CPU usage
2) often making poor decisions about what to reclaim

This work was mainly targeted toward improving those things, with an
eye toward interactive performance for browser workloads.

We have a few key tests we use for that, that measure tab switch times
and number of tab discards when under memory pressure, and this
approach significantly improves these (see Yu's data).

We do expect this approach will also be beneficial to cloud workloads,
and so are looking for people to try it out in their environments with
their favorite key tests or workloads.

Thanks,
Jesse

Re: [PATCH net 1/1] net: stmmac: set TxQ mode back to DCB after disabling CBS

[Jesse Brandeburg]

Song Yoong Siang wrote:

> From: Mohammad Athari Bin Ismail 
> 
> When disable CBS, mode_to_use parameter is not updated even the operation
> mode of Tx Queue is changed to Data Centre Bridging (DCB). Therefore,
> when tc_setup_cbs() function is called to re-enable CBS, the operation
> mode of Tx Queue remains at DCB, which causing CBS fails to work.
> 
> This patch updates the value of mode_to_use parameter to MTL_QUEUE_DCB
> after operation mode of Tx Queue is changed to DCB in stmmac_dma_qmode()
> callback function.
> 
> Fixes: 1f705bc61aee ("net: stmmac: Add support for CBS QDISC")
> Suggested-by: Gomes, Vinicius 
> Signed-off-by: Mohammad Athari Bin Ismail 
> Signed-off-by: Song, Yoong Siang 

Reviewed-by: Jesse Brandeburg 

Re: [net-next v3 00/14] Add Marvell CN10K support

[Jesse Brandeburg]

Geetha sowjanya wrote:

> v2-v3
> Reposting as a single thread.

FYI, it didn't work, suggest you try adding the git-send-email option
(via git-config)

sendemail.thread=true
sendemail.chainreplyto=false

And you can test locally by using first using git send-email to export
to mbox and checking for References and In-Reply-to headers. Then
sending for real.

Good luck!

Re: [PATCH RESEND v3 net-next 4/5] net: use the new dev_page_is_reusable() instead of private versions

[Jesse Brandeburg]

Alexander Lobakin wrote:

> Now we can remove a bunch of identical functions from the drivers and
> make them use common dev_page_is_reusable(). All {,un}likely() checks
> are omitted since it's already present in this helper.
> Also update some comments near the call sites.
> 
> Suggested-by: David Rientjes 
> Suggested-by: Jakub Kicinski 
> Cc: John Hubbard 
> Signed-off-by: Alexander Lobakin 

I don't know why it was missed in the series update, but:
Reviewed-by: Jesse Brandeburg 

Re: [PATCH net-next] net: Return the correct errno code

[Jesse Brandeburg]

Zheng Yongjun wrote:

> When kzalloc failed, should return ENOMEM rather than ENOBUFS.

All these patches have the same subject and description, couldn't they
just be part of a single series with a good cover letter?

I'm not saying make them a single patch, because that is bad for
bisection, but having them as a single series means we review related
changes at one time, and can comment on them as a group.

Re: [PATCH] octeontx2-af: remove unneeded semicolon

[Jesse Brandeburg]

Yang Li wrote:

> Eliminate the following coccicheck warning:
> ./drivers/net/ethernet/marvell/octeontx2/af/rvu_npc_fs.c:272:2-3:
> Unneeded semicolon
> ./drivers/net/ethernet/marvell/octeontx2/af/rvu_debugfs.c:1809:3-4:
> Unneeded semicolon
> ./drivers/net/ethernet/marvell/octeontx2/af/rvu_debugfs.c:1788:3-4:
> Unneeded semicolon
> ./drivers/net/ethernet/marvell/octeontx2/af/rvu.c:1326:2-3: Unneeded
> semicolon
> 
> Reported-by: Abaci Robot 
> Signed-off-by: Yang Li 
> ---
>  drivers/net/ethernet/marvell/octeontx2/af/rvu.c | 2 +-
>  drivers/net/ethernet/marvell/octeontx2/af/rvu_debugfs.c | 4 ++--
>  drivers/net/ethernet/marvell/octeontx2/af/rvu_npc_fs.c  | 2 +-
>  3 files changed, 4 insertions(+), 4 deletions(-)

Trivial patch, recommend net-next as it's not a critical fix, Yang,
please include the targeted tree when sending like [PATCH net-next]

otherwise, for net-next:
Reviewed-by: Jesse Brandeburg 

Re: [PATCH net] hv_netvsc: Reset the RSC count if NVSP_STAT_FAIL in netvsc_receive()

[Jesse Brandeburg]

Andrea Parri (Microsoft) wrote:

> Commit 44144185951a0f ("hv_netvsc: Add validation for untrusted Hyper-V
> values") added validation to rndis_filter_receive_data() (and
> rndis_filter_receive()) which introduced NVSP_STAT_FAIL-scenarios where
> the count is not updated/reset.  Fix this omission, and prevent similar
> scenarios from occurring in the future.
> 
> Reported-by: Juan Vazquez 
> Signed-off-by: Andrea Parri (Microsoft) 
> Cc: "David S. Miller" 
> Cc: Jakub Kicinski 
> Cc: net...@vger.kernel.org
> Fixes: 44144185951a0f ("hv_netvsc: Add validation for untrusted Hyper-V 
> values")

Reviewed-by: Jesse Brandeburg 

Re: [PATCH][next] net: hns3: remove redundant null check of an array

[Jesse Brandeburg]

Colin King wrote:

> From: Colin Ian King 
> 
> The null check of filp->f_path.dentry->d_iname is redundant because
> it is an array of DNAME_INLINE_LEN chars and cannot be a null. Fix
> this by removing the null check.
> 
> Addresses-Coverity: ("Array compared against 0")
> Fixes: 04987ca1b9b6 ("net: hns3: add debugfs support for tm nodes, priority 
> and qset info")
> Signed-off-by: Colin Ian King 

Reviewed-by: Jesse Brandeburg 

Re: [PATCH][next] net/mlx5e: Fix spelling mistake "Unknouwn" -> "Unknown"

[Jesse Brandeburg]

Colin King wrote:

> From: Colin Ian King 
> 
> There is a spelling mistake in a netdev_warn message. Fix it.
> 
> Signed-off-by: Colin Ian King 

Trivial patch, looks fine!

Reviewed-by: Jesse Brandeburg 

Re: [Patch v3 net-next 7/7] octeontx2-pf: ethtool physical link configuration

[Jesse Brandeburg]

Hariprasad Kelam wrote:

> From: Christina Jacob 
> 
> Register set_link_ksetting callback with driver such that
> link configurations parameters like advertised mode,speed, duplex
> and autoneg can be configured.
> 
> below command
> ethtool -s eth0 advertise 0x1 speed 10 duplex full autoneg on
> 
> Signed-off-by: Christina Jacob 
> Signed-off-by: Sunil Goutham 
> Signed-off-by: Hariprasad Kelam 

Reviewed-by: Jesse Brandeburg 

Re: [Patch v3 net-next 6/7] octeontx2-pf: ethtool physical link status

[Jesse Brandeburg]

Hariprasad Kelam wrote:

> From: Christina Jacob 
> 
> Register get_link_ksettings callback to get link status information
> from the driver. As virtual function (vf) shares same physical link
> same API is used for both the drivers and for loop back drivers
> simply returns the fixed values as its does not have physical link.
> 
> ethtool eth3
> Settings for eth3:
> Supported ports: [ ]
> Supported link modes:   10baseT/Half 10baseT/Full
> 100baseT/Half 100baseT/Full
> 1000baseT/Half 1000baseT/Full
> 1baseKR/Full
> 1000baseX/Full
> Supports auto-negotiation: No
> Supported FEC modes: BaseR RS
> Advertised link modes:  Not reported
> Advertised pause frame use: No
> Advertised auto-negotiation: No
> Advertised FEC modes: None
> 
> ethtool lbk0
> Settings for lbk0:
>   Speed: 10Mb/s
> Duplex: Full
> 
> Signed-off-by: Christina Jacob 
> Signed-off-by: Sunil Goutham 
> Signed-off-by: Hariprasad Kelam 

besides the slightly long lines, looks good.
Reviewed-by: Jesse Brandeburg 

Re: [Patch v3 net-next 5/7] octeontx2-af: advertised link modes support on cgx

[Jesse Brandeburg]

Hariprasad Kelam wrote:

> From: Christina Jacob 
> 
> CGX supports setting advertised link modes on physical link.
> This patch adds support to derive cgx mode from ethtool
> link mode and pass it to firmware to configure the same.
> 
> Signed-off-by: Christina Jacob 
> Signed-off-by: Sunil Goutham 
> Signed-off-by: Hariprasad Kelam 

Reviewed-by: Jesse Brandeburg 

Re: [Patch v3 net-next 4/7] octeontx2-af: Physical link configuration support

[Jesse Brandeburg]

Hariprasad Kelam wrote:

> From: Christina Jacob 
> 
> CGX LMAC, the physical interface support link configuration parameters
> like speed, auto negotiation, duplex  etc. Firmware saves these into
> memory region shared between firmware and this driver.
> 
> This patch adds mailbox handler set_link_mode, fw_data_get to
> configure and read these parameters.
> 
> Signed-off-by: Christina Jacob 
> Signed-off-by: Sunil Goutham 
> Signed-off-by: Hariprasad Kelam 

Reviewed-by: Jesse Brandeburg 

Re: [Patch v3 net-next 3/7] octeontx2-pf: ethtool fec mode support

[Jesse Brandeburg]

Hariprasad Kelam wrote:

> From: Christina Jacob 
> 
> Add ethtool support to configure fec modes baser/rs and
> support to fecth FEC stats from CGX as well PHY.
> 
> Configure fec mode
>   - ethtool --set-fec eth0 encoding rs/baser/off/auto
> Query fec mode
>   - ethtool --show-fec eth0
> 
> Signed-off-by: Christina Jacob 
> Signed-off-by: Sunil Goutham 
> Signed-off-by: Hariprasad Kelam 

Reviewed-by: Jesse Brandeburg 

Re: [Patch v3 net-next 2/7] octeontx2-af: Add new CGX_CMD to get PHY FEC statistics

[Jesse Brandeburg]

Hariprasad Kelam wrote:

> From: Felix Manlunas 
> 
> This patch adds support to fetch fec stats from PHY. The stats are
> put in the shared data struct fwdata.  A PHY driver indicates
> that it has FEC stats by setting the flag fwdata.phy.misc.has_fec_stats
> 
> Besides CGX_CMD_GET_PHY_FEC_STATS, also add CGX_CMD_PRBS and
> CGX_CMD_DISPLAY_EYE to enum cgx_cmd_id so that Linux's enum list is in sync
> with firmware's enum list.
> 
> Signed-off-by: Felix Manlunas 
> Signed-off-by: Christina Jacob 
> Signed-off-by: Sunil Kovvuri Goutham 
> Signed-off-by: Hariprasad Kelam 

Reviewed-by: Jesse Brandeburg 

Re: [Patch v3 net-next 1/7] octeontx2-af: forward error correction configuration

[Jesse Brandeburg]

Hariprasad Kelam wrote:

> From: Christina Jacob 
> 
> CGX block supports forward error correction modes baseR
> and RS. This patch adds support to set encoding mode
> and to read corrected/uncorrected block counters
> 
> Adds new mailbox handlers set_fec to configure encoding modes
> and fec_stats to read counters and also increase mbox timeout
> to accomdate firmware command response timeout.
> 
> Along with new CGX_CMD_SET_FEC command add other commands to
> sync with kernel enum list with firmware.
> 
> Signed-off-by: Christina Jacob 
> Signed-off-by: Sunil Goutham 
> Signed-off-by: Hariprasad Kelam 

Reviewed-by: Jesse Brandeburg 

Re: [Patch v3 net-next 0/7] ethtool support for fec and link configuration

[Jesse Brandeburg]

Hariprasad Kelam wrote:

> This series of patches add support for forward error correction(fec) and
> physical link configuration. Patches 1&2 adds necessary mbox handlers for fec
> mode configuration request and to fetch stats. Patch 3 registers driver
> callbacks for fec mode configuration and display. Patch 4&5 adds support of 
> mbox
> handlers for configuring link parameters like speed/duplex and autoneg etc.
> Patche 6&7 registers driver callbacks for physical link configuration.

For the series, in addition to the fact that Willem already replied to
the previous posting of v3 that it looked good.

Reviewed-by: Jesse Brandeburg 

Re: [PATCH v2 net-next 0/4] net: consolidate page_is_pfmemalloc() usage

[Jesse Brandeburg]

Alexander Lobakin wrote:

> page_is_pfmemalloc() is used mostly by networking drivers to test
> if a page can be considered for reusing/recycling.
> It doesn't write anything to the struct page itself, so its sole
> argument can be constified, as well as the first argument of
> skb_propagate_pfmemalloc().
> In Page Pool core code, it can be simply inlined instead.
> Most of the callers from NIC drivers were just doppelgangers of
> the same condition tests. Derive them into a new common function
> do deduplicate the code.

This is a useful cleanup! Thanks.

For the series:
Reviewed-by: Jesse Brandeburg 

Re: [PATCH v2 net-next 3/4] net: introduce common dev_page_is_reserved()

[Jesse Brandeburg]

Alexander Lobakin wrote:

> A bunch of drivers test the page before reusing/recycling for two
> common conditions:
>  - if a page was allocated under memory pressure (pfmemalloc page);
>  - if a page was allocated at a distant memory node (to exclude
>slowdowns).
> 
> Introduce and use a new common function for doing this and eliminate
> all functions-duplicates from drivers.
> 
> Suggested-by: David Rientjes 
> Signed-off-by: Alexander Lobakin 
> ---
>  drivers/net/ethernet/hisilicon/hns3/hns3_enet.c   | 10 ++
>  drivers/net/ethernet/intel/fm10k/fm10k_main.c |  9 ++---
>  drivers/net/ethernet/intel/i40e/i40e_txrx.c   | 15 +--
>  drivers/net/ethernet/intel/iavf/iavf_txrx.c   | 15 +--
>  drivers/net/ethernet/intel/ice/ice_txrx.c | 11 +--
>  drivers/net/ethernet/intel/igb/igb_main.c |  7 +--
>  drivers/net/ethernet/intel/igc/igc_main.c |  7 +--
>  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |  7 +--
>  drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c |  7 +--
>  drivers/net/ethernet/mellanox/mlx5/core/en_rx.c   |  7 +--
>  include/linux/skbuff.h| 15 +++
>  11 files changed, 27 insertions(+), 83 deletions(-)

For the patch, and esp. for the Intel drivers:
Reviewed-by: Jesse Brandeburg 

Re: [PATCH 1/1] ice: fix array overflow on receiving too many fragments for a packet

[Jesse Brandeburg]

Xiaohui Zhang wrote:

> From: Zhang Xiaohui 
> 
> If the hardware receives an oversized packet with too many rx fragments,
> skb_shinfo(skb)->frags can overflow and corrupt memory of adjacent pages.
> This becomes especially visible if it corrupts the freelist pointer of
> a slab page.

As I replied to the ionic patch, please justify this with how you found
it and how you reproduced a problem. Resend the patches as a series so
we can discuss them as one change.

> 
> Signed-off-by: Zhang Xiaohui 
> ---
>  drivers/net/ethernet/intel/ice/ice_txrx.c | 6 +-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_txrx.c 
> b/drivers/net/ethernet/intel/ice/ice_txrx.c
> index eae75260f..f0f034fa5 100644
> --- a/drivers/net/ethernet/intel/ice/ice_txrx.c
> +++ b/drivers/net/ethernet/intel/ice/ice_txrx.c
> @@ -823,8 +823,12 @@ ice_add_rx_frag(struct ice_ring *rx_ring, struct 
> ice_rx_buf *rx_buf,
>  
>   if (!size)
>   return;
> - skb_add_rx_frag(skb, skb_shinfo(skb)->nr_frags, rx_buf->page,
> + struct skb_shared_info *shinfo = skb_shinfo(skb);
> +
> + if (shinfo->nr_frags < ARRAY_SIZE(shinfo->frags)) {
> + skb_add_rx_frag(skb, shinfo, rx_buf->page,
>   rx_buf->page_offset, size, truesize);
> + }

The driver is using 2kB receive buffers, and can chain them together up
to a max receive size of 9126 bytes (or so), so how can we receive more
than 18 fragments? Please explain your logic

>  
>   /* page is being used so we must update the page offset */
>   ice_rx_buf_adjust_pg_offset(rx_buf, truesize);

Your patch doesn't compile. You must compile test and explain your
patches better.

  CC [M]  drivers/net/ethernet/intel/ice//ice_main.o
  CC [M]  drivers/net/ethernet/intel/ice//ice_controlq.o
  CC [M]  drivers/net/ethernet/intel/ice//ice_common.o
  CC [M]  drivers/net/ethernet/intel/ice//ice_nvm.o
  CC [M]  drivers/net/ethernet/intel/ice//ice_switch.o
  CC [M]  drivers/net/ethernet/intel/ice//ice_sched.o
  CC [M]  drivers/net/ethernet/intel/ice//ice_base.o
  CC [M]  drivers/net/ethernet/intel/ice//ice_lib.o
  CC [M]  drivers/net/ethernet/intel/ice//ice_txrx_lib.o
  CC [M]  drivers/net/ethernet/intel/ice//ice_txrx.o
drivers/net/ethernet/intel/ice//ice_txrx.c: In function ‘ice_add_rx_frag’:
drivers/net/ethernet/intel/ice//ice_txrx.c:829:2: warning: ISO C90 forbids 
mixed declarations and code [-Wdeclaration-after-statement]
  829 |  struct skb_shared_info *shinfo = skb_shinfo(skb);
  |  ^~
drivers/net/ethernet/intel/ice//ice_txrx.c:832:24: warning: passing argument 2 
of ‘skb_add_rx_frag’ makes integer from pointer without a cast 
[-Wint-conversion]
  832 |   skb_add_rx_frag(skb, shinfo, rx_buf->page,
  |^~
  ||
  |struct skb_shared_info *
In file included from ./include/linux/if_ether.h:19,
 from ./include/uapi/linux/ethtool.h:19,
 from ./include/linux/ethtool.h:18,
 from ./include/linux/netdevice.h:37,
 from ./include/trace/events/xdp.h:8,
 from ./include/linux/bpf_trace.h:5,
 from drivers/net/ethernet/intel/ice//ice_txrx.c:8:
./include/linux/skbuff.h:2182:47: note: expected ‘int’ but argument is of type 
‘struct skb_shared_info *’
 2182 | void skb_add_rx_frag(struct sk_buff *skb, int i, struct page *page, int 
off,
  |   ^

Re: [PATCH 1/1] ionic: fix array overflow on receiving too many fragments for a packet

[Jesse Brandeburg]

Xiaohui Zhang wrote:

> From: Zhang Xiaohui 
> 
> If the hardware receives an oversized packet with too many rx fragments,
> skb_shinfo(skb)->frags can overflow and corrupt memory of adjacent pages.
> This becomes especially visible if it corrupts the freelist pointer of
> a slab page.
> 
> Signed-off-by: Zhang Xiaohui 

Hi, thanks for your patch.

It appears this is a part of a series of patches (at least this one and
one to the ice driver) - please send as one series, with a cover letter
explanation.

Please justify how this is a bug and how this is found / reproduced.

I'll respond separately to the ice driver patch as I don't know this
hardware and it's limits, but I suspect that you've tried to fix a bug
where there was none. (It seems like something a code scanner might find
and be confused about)

> ---
>  drivers/net/ethernet/pensando/ionic/ionic_txrx.c | 6 +-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_txrx.c 
> b/drivers/net/ethernet/pensando/ionic/ionic_txrx.c
> index 169ac4f54..a3e274c65 100644
> --- a/drivers/net/ethernet/pensando/ionic/ionic_txrx.c
> +++ b/drivers/net/ethernet/pensando/ionic/ionic_txrx.c
> @@ -102,8 +102,12 @@ static struct sk_buff *ionic_rx_frags(struct ionic_queue 
> *q,
>  
>   dma_unmap_page(dev, dma_unmap_addr(page_info, dma_addr),
>  PAGE_SIZE, DMA_FROM_DEVICE);
> - skb_add_rx_frag(skb, skb_shinfo(skb)->nr_frags,
> + struct skb_shared_info *shinfo = skb_shinfo(skb);

you can't declare variables in the middle of a code flow in C, did you
compile this?

> +
> + if (shinfo->nr_frags < ARRAY_SIZE(shinfo->frags)) {
> + skb_add_rx_frag(skb, shinfo->nr_frags,
>   page_info->page, 0, frag_len, PAGE_SIZE);
> + }
>   page_info->page = NULL;
>   page_info++;
>   i--;

Re: [PATCH V4 net-next 0/4] net: hns3: updates for -next

[Jesse Brandeburg]

Huazhong Tan wrote:

> There are several updates relating to the interrupt coalesce for
> the HNS3 ethernet driver.
> 
> #1 adds support for QL(quantity limiting, interrupt coalesce
>based on the frame quantity).
> #2 queries the maximum value of GL from the firmware instead of
>a fixed value in code.
> #3 adds support for 1us unit GL(gap limiting, interrupt coalesce
>based on the gap time).
> #4 renames gl_adapt_enable in struct hns3_enet_coalesce to fit
>its new usage.
> 
> change log:
> V4 - remove #5~#10 from this series, which needs more discussion.
> V3 - fix a typo error in #1 reported by Jakub Kicinski.
>  rewrite #9 commit log.
>  remove #11 from this series.
> V2 - reorder #2 & #3 to fix compiler error.
>  fix some checkpatch warnings in #10 & #11.


For the series:
Reviewed-by: Jesse Brandeburg 

Re: [PATCH] x86/gpu: add JSL stolen memory support

[Jesse Barnes]

On Thu, Nov 19, 2020 at 11:19 AM Bjorn Helgaas  wrote:
>
> [+cc Jesse]
>
> On Thu, Nov 19, 2020 at 10:37:10AM +0100, Daniel Vetter wrote:
> > On Thu, Nov 19, 2020 at 12:14 AM Bjorn Helgaas  wrote:
> > > On Wed, Nov 18, 2020 at 10:57:26PM +0100, Daniel Vetter wrote:
> > > > On Wed, Nov 18, 2020 at 5:02 PM Bjorn Helgaas  
> > > > wrote:
> > > > > On Fri, Nov 06, 2020 at 10:39:16AM +0100, Daniel Vetter wrote:
> > > > > > On Thu, Nov 5, 2020 at 3:17 PM Bjorn Helgaas  
> > > > > > wrote:
> > > > > > > On Thu, Nov 05, 2020 at 11:46:06AM +0200, Joonas Lahtinen wrote:
> > > > > > > > Quoting Bjorn Helgaas (2020-11-04 19:35:56)
> > > > > > > > > [+cc Jani, Joonas, Rodrigo, David, Daniel]
> > > > > > > > >
> > > > > > > > > On Wed, Nov 04, 2020 at 05:35:06PM +0530, Tejas Upadhyay 
> > > > > > > > > wrote:
> > > > > > > > > > JSL re-uses the same stolen memory as ICL and EHL.
> > > > > > > > > >
> > > > > > > > > > Cc: Lucas De Marchi 
> > > > > > > > > > Cc: Matt Roper 
> > > > > > > > > > Signed-off-by: Tejas Upadhyay 
> > > > > > > > > > 
> > > > > > > > >
> > > > > > > > > I don't plan to do anything with this since previous similar 
> > > > > > > > > patches
> > > > > > > > > have gone through some other tree, so this is just kibitzing.
> > > > > > > > >
> > > > > > > > > But the fact that we have this long list of Intel devices [1] 
> > > > > > > > > that
> > > > > > > > > constantly needs updates [2] is a hint that something is 
> > > > > > > > > wrong.
> > > > > > > >
> > > > > > > > We add an entry for every new integrated graphics platform. 
> > > > > > > > Once the
> > > > > > > > platform is added, there have not been changes lately.
> > > > > > > >
> > > > > > > > > IIUC the general idea is that we need to discover Intel gfx 
> > > > > > > > > memory by
> > > > > > > > > looking at device-dependent config space and add it to the 
> > > > > > > > > E820 map.
> > > > > > > > > Apparently the quirks discover this via PCI config registers 
> > > > > > > > > like
> > > > > > > > > I830_ESMRAMC, I845_ESMRAMC, etc, and tell the driver about it 
> > > > > > > > > via the
> > > > > > > > > global "intel_graphics_stolen_res"?
> > > > > > > >
> > > > > > > > We discover what is called the graphics data stolen memory. It 
> > > > > > > > is regular
> > > > > > > > system memory range that is not CPU accessible. It is 
> > > > > > > > accessible by the
> > > > > > > > integrated graphics only.
> > > > > > > >
> > > > > > > > See: 
> > > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/arch/x86/kernel/early-quirks.c?h=v5.10-rc2=814c5f1f52a4beb3710317022acd6ad34fc0b6b9
> > > > > > > >
> > > > > > > > > That's not the way this should work.  There should some 
> > > > > > > > > generic, non
> > > > > > > > > device-dependent PCI or ACPI method to discover the memory 
> > > > > > > > > used, or at
> > > > > > > > > least some way to do it in the driver instead of early arch 
> > > > > > > > > code.
> > > > > > > >
> > > > > > > > It's used by the early BIOS/UEFI code to set up initial 
> > > > > > > > framebuffer.
> > > > > > > > Even if i915 driver is never loaded, the memory ranges still 
> > > > > > > > need to
> > > > > > > > be fixed. They source of the problem is that the OEM BIOS which 
> > > > > > > > are
> > > > > > > > not under our control get the p

Re: [PATCH net] iavf: fix error return code in iavf_init_get_resources()

[Jesse Brandeburg]

Zhang Changzhong wrote:

> Fix to return a negative error code from the error handling
> case instead of 0, as done elsewhere in this function.
> 
> Fixes: b66c7bc1cd4d ("iavf: Refactor init state machine")
> Reported-by: Hulk Robot 
> Signed-off-by: Zhang Changzhong 

Thanks!

Reviewed-by: Jesse Brandeburg 

Re: [PATCH 1/2] tools/include: Update if_link.h and netlink.h

[Jesse Brandeburg]

Jakub Kicinski wrote:

> On Fri, 16 Oct 2020 14:23:48 -0700 Jesse Brandeburg wrote:
> > > These are tested to be the latest as part of the tools/lib/bpf build.  
> > 
> > But you didn't mention why you're making these changes, and you're
> > removing a lot of comments without explaining why/where there might be
> > a replacement or why the comments are useless. I now see that you're
> > adding actual kdoc which is good, except for the part where
> > you don't put kdoc on all the structures.
> 
> Note that he's just syncing the uAPI headers to tools/
> 
> The source of the change is here:
> 
> 78a3ea555713 ("net: remove comments on struct rtnl_link_stats")
> 0db0c34cfbc9 ("net: tighten the definition of interface statistics")


Thanks Kuba, I'm not trying to be a hard ass, but the commit message
didn't say why he's making the change, and if I bisect back to this
and see "sync" as the commit message, I think I'd be stuck chasing
"sync to what?"

I guess that his changelog could just say what you said?

Proposed:
Sync the uAPI headers so that userspace and the kernel match. These
changes match the updates to the files in the tools directory that were
already updated by commits:
78a3ea555713 ("net: remove comments on struct rtnl_link_stats")
0db0c34cfbc9 ("net: tighten the definition of interface statistics")

Re: [PATCH net-next v2 0/9] net: ethernet: ti: am65-cpsw: add multi port support in mac-only mode

[Jesse Brandeburg]

Grygorii Strashko wrote:

> Hi
> 
> This series adds multi-port support in mac-only mode (multi MAC mode) to TI
> AM65x CPSW driver in preparation for enabling support for multi-port devices,
> like Main CPSW0 on K3 J721E SoC or future CPSW3g on K3 AM64x SoC.

For the series
Reviewed-by: Jesse Brandeburg 

Re: [PATCH 1/2] tools/include: Update if_link.h and netlink.h

[Jesse Brandeburg]

Hi Ian,

Ian Rogers wrote:

> These are tested to be the latest as part of the tools/lib/bpf build.

But you didn't mention why you're making these changes, and you're
removing a lot of comments without explaining why/where there might be
a replacement or why the comments are useless. I now see that you're
adding actual kdoc which is good, except for the part where
you don't put kdoc on all the structures.

> 
> Signed-off-by: Ian Rogers 
> ---
>  tools/include/uapi/linux/if_link.h | 269 +
>  tools/include/uapi/linux/netlink.h | 107 
>  2 files changed, 342 insertions(+), 34 deletions(-)
> 
> diff --git a/tools/include/uapi/linux/if_link.h 
> b/tools/include/uapi/linux/if_link.h
> index 781e482dc499..c4b23f06f69e 100644
> --- a/tools/include/uapi/linux/if_link.h
> +++ b/tools/include/uapi/linux/if_link.h
> @@ -7,24 +7,23 @@
>  
>  /* This struct should be in sync with struct rtnl_link_stats64 */

Maybe you should put a "definitions are available in the comment above
rtnl_link_stats64" comment here?

>  struct rtnl_link_stats {
> - __u32   rx_packets; /* total packets received   */
> - __u32   tx_packets; /* total packets transmitted*/
> - __u32   rx_bytes;   /* total bytes received */
> - __u32   tx_bytes;   /* total bytes transmitted  */
> - __u32   rx_errors;  /* bad packets received */
> - __u32   tx_errors;  /* packet transmit problems */
> - __u32   rx_dropped; /* no space in linux buffers*/
> - __u32   tx_dropped; /* no space available in linux  */
> - __u32   multicast;  /* multicast packets received   */
> + __u32   rx_packets;
> + __u32   tx_packets;
> + __u32   rx_bytes;

Why is removing all the comments useful? You didn't make any useful
change here.

> + __u32   tx_bytes;
> + __u32   rx_errors;
> + __u32   tx_errors;
> + __u32   rx_dropped;
> + __u32   tx_dropped;
> + __u32   multicast;
>   __u32   collisions;
> -
>   /* detailed rx_errors: */
>   __u32   rx_length_errors;
> - __u32   rx_over_errors; /* receiver ring buff overflow  */
> - __u32   rx_crc_errors;  /* recved pkt with crc error*/
> - __u32   rx_frame_errors;/* recv'd frame alignment error */
> - __u32   rx_fifo_errors; /* recv'r fifo overrun  */
> - __u32   rx_missed_errors;   /* receiver missed packet   */
> + __u32   rx_over_errors;
> + __u32   rx_crc_errors;

Same comment as above.

> + __u32   rx_frame_errors;
> + __u32   rx_fifo_errors;
> + __u32   rx_missed_errors;
>  
>   /* detailed tx_errors */
>   __u32   tx_aborted_errors;
> @@ -37,29 +36,200 @@ struct rtnl_link_stats {
>   __u32   rx_compressed;
>   __u32   tx_compressed;
>  
> - __u32   rx_nohandler;   /* dropped, no handler found*/
> + __u32   rx_nohandler;

And here too!

>  };
>  
> -/* The main device statistics structure */
> +/**
> + * struct rtnl_link_stats64 - The main device statistics structure.
> + *
> + * @rx_packets: Number of good packets received by the interface.
> + *   For hardware interfaces counts all good packets received from the device
> + *   by the host, including packets which host had to drop at various stages
> + *   of processing (even in the driver).

Oh, I see now. A good commit message would have prevented me burping on
this so much.

> + *
> + * @tx_packets: Number of packets successfully transmitted.
> + *   For hardware interfaces counts packets which host was able to 
> successfully
> + *   hand over to the device, which does not necessarily mean that packets
> + *   had been successfully transmitted out of the device, only that device
> + *   acknowledged it copied them out of host memory.
> + *
> + * @rx_bytes: Number of good received bytes, corresponding to @rx_packets.
> + *
> + *   For IEEE 802.3 devices should count the length of Ethernet Frames
> + *   excluding the FCS.
> + *
> + * @tx_bytes: Number of good transmitted bytes, corresponding to @tx_packets.
> + *
> + *   For IEEE 802.3 devices should count the length of Ethernet Frames
> + *   excluding the FCS.
> + *
> + * @rx_errors: Total number of bad packets received on this network device.
> + *   This counter must include events counted by @rx_length_errors,
> + *   @rx_crc_errors, @rx_frame_errors and other errors not otherwise
> + *   counted.
> + *
> + * @tx_errors: Total number of transmit problems.
> + *   This counter must include events counter by @tx_aborted_errors,
> + *   @tx_carrier_errors, @tx_fifo_errors, @tx_heartbeat_errors,
> + *   @tx_window_errors and other errors not otherwise counted.
> + *
> + * @rx_dropped: Number of packets received but not processed,
> + *   e.g. due to lack of resources or unsupported protocol.
> + *   For hardware interfaces this counter 

Re: [RFC][Patch v1 2/3] i40e: limit msix vectors based on housekeeping CPUs

[Jesse Brandeburg]

Nitesh Narayan Lal wrote:

> In a realtime environment, it is essential to isolate unwanted IRQs from
> isolated CPUs to prevent latency overheads. Creating MSIX vectors only
> based on the online CPUs could lead to a potential issue on an RT setup
> that has several isolated CPUs but a very few housekeeping CPUs. This is
> because in these kinds of setups an attempt to move the IRQs to the
> limited housekeeping CPUs from isolated CPUs might fail due to the per
> CPU vector limit. This could eventually result in latency spikes because
> of the IRQ threads that we fail to move from isolated CPUs.
> 
> This patch prevents i40e to add vectors only based on available
> housekeeping CPUs by using num_housekeeping_cpus().
> 
> Signed-off-by: Nitesh Narayan Lal 

The driver changes are straightforward, but this isn't the only driver
with this issue, right?  I'm sure ixgbe and ice both have this problem
too, you should fix them as well, at a minimum, and probably other
vendors drivers:

$ rg -c --stats num_online_cpus drivers/net/ethernet
...
50 files contained matches

for this patch i40e
Acked-by: Jesse Brandeburg 

Re: [RFC][Patch v1 1/3] sched/isolation: API to get num of hosekeeping CPUs

[Jesse Brandeburg]

Nitesh Narayan Lal wrote:

> Introduce a new API num_housekeeping_cpus(), that can be used to retrieve
> the number of housekeeping CPUs by reading an atomic variable
> __num_housekeeping_cpus. This variable is set from housekeeping_setup().
> 
> This API is introduced for the purpose of drivers that were previously
> relying only on num_online_cpus() to determine the number of MSIX vectors
> to create. In an RT environment with large isolated but a fewer
> housekeeping CPUs this was leading to a situation where an attempt to
> move all of the vectors corresponding to isolated CPUs to housekeeping
> CPUs was failing due to per CPU vector limit.
> 
> If there are no isolated CPUs specified then the API returns the number
> of all online CPUs.
> 
> Signed-off-by: Nitesh Narayan Lal 
> ---
>  include/linux/sched/isolation.h |  7 +++
>  kernel/sched/isolation.c| 23 +++
>  2 files changed, 30 insertions(+)

I'm not a scheduler expert, but a couple comments follow.

> 
> diff --git a/include/linux/sched/isolation.h b/include/linux/sched/isolation.h
> index cc9f393e2a70..94c25d956d8a 100644
> --- a/include/linux/sched/isolation.h
> +++ b/include/linux/sched/isolation.h
> @@ -25,6 +25,7 @@ extern bool housekeeping_enabled(enum hk_flags flags);
>  extern void housekeeping_affine(struct task_struct *t, enum hk_flags flags);
>  extern bool housekeeping_test_cpu(int cpu, enum hk_flags flags);
>  extern void __init housekeeping_init(void);
> +extern unsigned int num_housekeeping_cpus(void);
>  
>  #else
>  
> @@ -46,6 +47,12 @@ static inline bool housekeeping_enabled(enum hk_flags 
> flags)
>  static inline void housekeeping_affine(struct task_struct *t,
>  enum hk_flags flags) { }
>  static inline void housekeeping_init(void) { }
> +
> +static unsigned int num_housekeeping_cpus(void)
> +{
> + return num_online_cpus();
> +}
> +
>  #endif /* CONFIG_CPU_ISOLATION */
>  
>  static inline bool housekeeping_cpu(int cpu, enum hk_flags flags)
> diff --git a/kernel/sched/isolation.c b/kernel/sched/isolation.c
> index 5a6ea03f9882..7024298390b7 100644
> --- a/kernel/sched/isolation.c
> +++ b/kernel/sched/isolation.c
> @@ -13,6 +13,7 @@ DEFINE_STATIC_KEY_FALSE(housekeeping_overridden);
>  EXPORT_SYMBOL_GPL(housekeeping_overridden);
>  static cpumask_var_t housekeeping_mask;
>  static unsigned int housekeeping_flags;
> +static atomic_t __num_housekeeping_cpus __read_mostly;
>  
>  bool housekeeping_enabled(enum hk_flags flags)
>  {
> @@ -20,6 +21,27 @@ bool housekeeping_enabled(enum hk_flags flags)
>  }
>  EXPORT_SYMBOL_GPL(housekeeping_enabled);
>  
> +/*

use correct kdoc style, and you get free documentation from your source
(you're so close!)

should be (note the first line and the function title line change to
remove parens:
/**
 * num_housekeeping_cpus - Read the number of housekeeping CPUs.
 *
 * This function returns the number of available housekeeping CPUs
 * based on __num_housekeeping_cpus which is of type atomic_t
 * and is initialized at the time of the housekeeping setup.
 */

> + * num_housekeeping_cpus() - Read the number of housekeeping CPUs.
> + *
> + * This function returns the number of available housekeeping CPUs
> + * based on __num_housekeeping_cpus which is of type atomic_t
> + * and is initialized at the time of the housekeeping setup.
> + */
> +unsigned int num_housekeeping_cpus(void)
> +{
> + unsigned int cpus;
> +
> + if (static_branch_unlikely(_overridden)) {
> + cpus = atomic_read(&__num_housekeeping_cpus);
> + /* We should always have at least one housekeeping CPU */
> + BUG_ON(!cpus);

you need to crash the kernel because of this? maybe a WARN_ON? How did
the global even get set to the bad value? It's going to blame the poor
caller for this in the trace, but the caller likely had nothing to do
with setting the value incorrectly!

> + return cpus;
> + }
> + return num_online_cpus();
> +}
> +EXPORT_SYMBOL_GPL(num_housekeeping_cpus);

Re: [PATCH net-next 0/3] Fix some kernel-doc warnings for i40e

[Jesse Brandeburg]

Wang Hai wrote:

> Wang Hai (3):
>   i40e: Fix some kernel-doc warnings in i40e_client.c
>   i40e: Fix some kernel-doc warnings in i40e_common.c
>   i40e: Fix a kernel-doc warning in i40e_ptp.c
> 
>  drivers/net/ethernet/intel/i40e/i40e_client.c | 2 --
>  drivers/net/ethernet/intel/i40e/i40e_common.c | 2 --
>  drivers/net/ethernet/intel/i40e/i40e_ptp.c| 1 -
>  3 files changed, 5 deletions(-)
> 


Please see my patchset [1]: I've already fixed all of these and many
others.

In fact, before you continue, I have a whole set done making the entire
drivers/net/ethernet directory compile cleanly with W=1, that I'm about
to send, but they depend on [1]

[1]
https://patchwork.ozlabs.org/project/intel-wired-lan/list/?series==189

Re: [Intel-wired-lan] VRRP not working on i40e X722 S2600WFT

[Jesse Brandeburg]

Lennart Sorensen wrote:

> On Thu, Aug 27, 2020 at 02:30:39PM -0400, Lennart Sorensen wrote:
> > I have hit a new problem with the X722 chipset (Intel R1304WFT server).
> > VRRP simply does not work.
> > 
> > When keepalived registers a vmac interface, and starts transmitting
> > multicast packets with the vrp message, it never receives those packets
> > from the peers, so all nodes think they are the master.  tcpdump shows
> > transmits, but no receives.  If I stop keepalived, which deletes the
> > vmac interface, then I start to receive the multicast packets from the
> > other nodes.  Even in promisc mode, tcpdump can't see those packets.
> > 
> > So it seems the hardware is dropping all packets with a source mac that
> > matches the source mac of the vmac interface, even when the destination
> > is a multicast address that was subcribed to.  This is clearly not
> > proper behaviour.

Thanks for the report Lennart, I understand your frustration, as this
should probably work without user configuration.

However, please give this command a try:
ethtool --set-priv-flags ethX disable-source-pruning on


> > I tried a stock 5.8 kernel to check if a driver update helped, and updated
> > the nvm firware to the latest 4.10 (which appears to be over a year old),
> > and nothing changes the behaviour at all.
> > 
> > Seems other people have hit this problem too:
> > http://mails.dpdk.org/archives/users/2018-May/003128.html
> > 
> > Unless someone has a way to fix this, we will have to change away from
> > this hardware very quickly.  The IPsec NAT RSS defect we could tolerate
> > although didn't like, while this is just unworkable.
> > 
> > Quite frustrated by this.  Intel network hardware was always great,
> > how did the X722 make it out in this state.
> 
> Another case with the same problem on an X710:
> 
> https://www.talkend.net/post/13256.html

I don't know how to reply to this other thread, but it is about DPDK,
which would require a code change or further investigation to issue the
right command to the hardware to disable source pruning.

Re: [net v2 PATCH 1/2] net: ethernet: ti: cpsw: fix clean up of vlan mc entries for host port

[Jesse Brandeburg]

Murali Karicheri wrote:

> To flush the vid + mc entries from ALE, which is required when a VLAN
> interface is removed, driver needs to call cpsw_ale_flush_multicast()
> with ALE_PORT_HOST for port mask as these entries are added only for
> host port. Without this, these entries remain in the ALE table even
> after removing the VLAN interface. cpsw_ale_flush_multicast() calls
> cpsw_ale_flush_mcast which expects a port mask to do the job.
> 
> Signed-off-by: Murali Karicheri 

Patch makes sense. Please resend with a Fixes: tag.

Re: [net v2 PATCH 2/2] net: ethernet: ti: cpsw_new: fix clean up of vlan mc entries for host port

[Jesse Brandeburg]

Murali Karicheri wrote:

> To flush the vid + mc entries from ALE, which is required when a VLAN
> interface is removed, driver needs to call cpsw_ale_flush_multicast()
> with ALE_PORT_HOST for port mask as these entries are added only for
> host port. Without this, these entries remain in the ALE table even
> after removing the VLAN interface. cpsw_ale_flush_multicast() calls
> cpsw_ale_flush_mcast which expects a port mask to do the job.
> 
> Signed-off-by: Murali Karicheri 

Patch looks good but please resend with a Fixes: tag.

Re: [PATCH] x86/pci: fix intel_mid_pci.c build error when ACPI is not enabled

[Jesse Barnes]

On Wed, Aug 19, 2020 at 9:08 PM Randy Dunlap  wrote:
>
> On 8/13/20 1:55 PM, Andy Shevchenko wrote:
> > On Thu, Aug 13, 2020 at 11:31 PM Arjan van de Ven  
> > wrote:
> >> On 8/13/2020 12:58 PM, Randy Dunlap wrote:
> >>> From: Randy Dunlap 
> >>>
> >>> Fix build error when CONFIG_ACPI is not set/enabled by adding
> >>> the header file  which contains a stub for the function
> >>> in the build error.
> >>>
> >>> ../arch/x86/pci/intel_mid_pci.c: In function ‘intel_mid_pci_init’:
> >>> ../arch/x86/pci/intel_mid_pci.c:303:2: error: implicit declaration of 
> >>> function ‘acpi_noirq_set’; did you mean ‘acpi_irq_get’? 
> >>> [-Werror=implicit-function-declaration]
> >>>acpi_noirq_set();
> >
> > Reviewed-by: Andy Shevchenko 
> > Thanks!
>
> also:
> Reviewed-by: Jesse Barnes 
>
> >
> >>> Signed-off-by: Randy Dunlap 
> >>> Cc: Jacob Pan 
> >>> Cc: Len Brown 
> >>> Cc: Bjorn Helgaas 
> >>> Cc: Jesse Barnes 
> >>> Cc: Arjan van de Ven 
> >>> Cc: linux-...@vger.kernel.org
> >>> ---
> >>> Found in linux-next, but applies to/exists in mainline also.
> >>>
> >>> Alternative.1: X86_INTEL_MID depends on ACPI
> >>> Alternative.2: drop X86_INTEL_MID support
> >>
> >> at this point I'd suggest Alternative 2; the products that needed that 
> >> (past tense, that technology
> >> is no longer need for any newer products) never shipped in any form where 
> >> a 4.x or 5.x kernel could
> >> work, and they are also all locked down...
> >
> > This is not true. We have Intel Edison which runs nicely on vanilla
> > (not everything, some is still requiring a couple of patches, but most
> > of it works out-of-the-box).
> >
> > And for the record, I have been working on removing quite a pile of
> > code (~13kLOCs to the date IIRC) in MID area. Just need some time to
> > fix Edison watchdog for that.
>
>
> I didn't see a consensus on this patch, although Andy says it's still needed,
> so it shouldn't be removed (yet). Maybe his big removal patch can remove it
> later. For now can we just fix the build error?


Yeah I think it makes sense to land it.  Doesn't get in the way of a
future removal and fixes a build error in the meantime.

Jesse

Re: NETDEV WATCHDOG: WARNING: at net/sched/sch_generic.c:442 dev_watchdog

[Jesse Brandeburg]

Steven Rostedt wrote:

> On Wed, 19 Aug 2020 17:01:06 +0530
> Naresh Kamboju  wrote:
> 
> > kernel warning noticed on x86_64 while running LTP tracing 
> > ftrace-stress-test
> > case. started noticing on the stable-rc linux-5.8.y branch.
> > 
> > This device booted with KASAN config and DYNAMIC tracing configs and more.
> > This reported issue is not easily reproducible.
> > 
> > metadata:
> >   git branch: linux-5.8.y
> >   git repo: 
> > https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable-rc.git
> >   git commit: ad8c735b1497520df959f675718f39dca8cb8019
> >   git describe: v5.8.2
> >   make_kernelversion: 5.8.2
> >   kernel-config:
> > https://builds.tuxbuild.com/bOz0eAwkcraRiWALTW9D3Q/kernel.config
> > 
> > 
> > [   88.139387] Scheduler tracepoints stat_sleep, stat_iowait,
> > stat_blocked and stat_runtime require the kernel parameter
> > schedstats=enable or kernel.sched_schedstats=1
> > [   88.139387] Scheduler tracepoints stat_sleep, stat_iowait,
> > stat_blocked and stat_runtime require the kernel parameter
> > schedstats=enable or kernel.sched_schedstats=1
> > [  107.507991] [ cut here ]
> > [  107.513103] NETDEV WATCHDOG: eth0 (igb): transmit queue 2 timed out
> > [  107.519973] WARNING: CPU: 1 PID: 331 at net/sched/sch_generic.c:442
> > dev_watchdog+0x4c7/0x4d0
> > [  107.528907] Modules linked in: x86_pkg_temp_thermal
> > [  107.534541] CPU: 1 PID: 331 Comm: systemd-journal Not tainted 5.8.2 #1
> > [  107.541480] Hardware name: Supermicro SYS-5019S-ML/X11SSH-F, BIOS
> > 2.2 05/23/2018
> > [  107.549314] RIP: 0010:dev_watchdog+0x4c7/0x4d0
> > [  107.554226] Code: ff ff 48 8b 5d c8 c6 05 6d f7 94 01 01 48 89 df
> > e8 9e b4 f8 ff 44 89 e9 48 89 de 48 c7 c7 20 49 51 9c 48 89 c2 e8 91
> > 7e e9 fe <0f> 0b e9 03 ff ff ff 66 90 e8 9b 23 db fe 55 48 89 e5 41 57
> 
> I've triggered this myself in my testing, and I assumed that adding the
> overhead of tracing and here KASAN too, made some watchdog a bit
> unhappy. By commenting out the warning, I've seen no ill effects.
> 
> Perhaps this is something we need to dig a bit deeper into.

Looked into it a little, igb uses a timeout of 5 seconds, and the stack
prints the warning if we haven't completed the transmit in that time.

What I don't understand in the stack trace is this:
> > [  107.654661] Call Trace:
> > [  107.657735]  
> > [  107.663155]  ? ftrace_graph_caller+0xc0/0xc0
> > [  107.667929]  call_timer_fn+0x3b/0x1b0
> > [  107.672238]  ? netif_carrier_off+0x70/0x70
> > [  107.61]  ? netif_carrier_off+0x70/0x70
> > [  107.682656]  ? ftrace_graph_caller+0xc0/0xc0
> > [  107.687379]  run_timer_softirq+0x3e8/0xa10
> > [  107.694653]  ? call_timer_fn+0x1b0/0x1b0
> > [  107.699382]  ? trace_event_raw_event_softirq+0xdd/0x150
> > [  107.706768]  ? ring_buffer_unlock_commit+0xf5/0x210
> > [  107.712213]  ? call_timer_fn+0x1b0/0x1b0
> > [  107.716625]  ? __do_softirq+0x155/0x467


If the carrier was turned off by something, that could cause the stack
to timeout since it appears the driver didn't call this itself after
finishing all transmits like it normally would have.

Is the trace above correct? Usually the ? indicate unsure backtrace due
to missing symbols, right?

Re: [PATCH] qed_main: Remove unnecessary cast in kfree

[Jesse Brandeburg]

On Tue, 18 Aug 2020 09:10:56 +
Xu Wang  wrote:

> Remove unnecassary casts in the argument to kfree.
> 
> Signed-off-by: Xu Wang 

You seem to have several of these patches, they should be sent in a
series with the series patch subject (for example):
[PATCH net-next 0/n] fix up casts on kfree

Did you use a coccinelle script to find these? 

They could all have Fixes tags. I'd resend the whole bunch as a series.

Since this has no functional change, you could mention that in the
series commit message text.

Jesse

Re: [PATCH] net/bluetooth/hidp/sock.c: add CAP_NET_RAW check.

[Jesse Brandeburg]

On Tue, 18 Aug 2020 16:21:03 +0800
Qingyu Li  wrote:

> When creating a raw PF_BLUETOOTH socket,
> CAP_NET_RAW needs to be checked first.
> 
> Signed-off-by: Qingyu Li 

Please see my replies to your previous patches.

Re: [PATCH] net/bluetooth/cmtp/sock.c: add CAP_NET_RAW check.

[Jesse Brandeburg]

On Tue, 18 Aug 2020 16:15:55 +0800
Qingyu Li  wrote:

> When creating a raw PF_BLUETOOTH socket,
> CAP_NET_RAW needs to be checked first.

Please see my previous replies.

Re: [PATCH] net/bluetooth/bnep/sock.c: add CAP_NET_RAW check.

[Jesse Brandeburg]

On Tue, 18 Aug 2020 16:07:03 +0800
Qingyu Li  wrote:

> When creating a raw PF_BLUETOOTH socket,
> CAP_NET_RAW needs to be checked first.
> 

These changes should be part of a series (patch 0,1,2 at least), and all
my replies on your other patch apply to this one as well.

Re: [PATCH] net/bluetooth/hci_sock.c: add CAP_NET_RAW check.

[Jesse Brandeburg]

On Tue, 18 Aug 2020 15:56:48 +0800
Qingyu Li  wrote:

> When creating a raw PF_BLUETOOTH socket,
> CAP_NET_RAW needs to be checked first.
> 

Thanks for the patch! Your subject doesn't need to end in a period. In
your commit message, I can guess why you'd want this patch, but your
commit message should include more info about why the kernel wants this
patch included. Especially since this is a user visible change and
likely a fix of a bug. Please review:
https://www.kernel.org/doc/html/latest/networking/netdev-FAQ.html
specifically:
https://www.kernel.org/doc/html/latest/networking/netdev-FAQ.html#q-any-other-tips-to-help-ensure-my-net-net-next-patch-gets-ok-d

This looks like a fix, please add a Fixes tag.

Re: [PATCH 12/30] net: wireless: cisco: airo: Fix a myriad of coding style issues

[Jesse Brandeburg]

On Mon, 17 Aug 2020 16:27:01 +0300
Kalle Valo  wrote:

> I was surprised to see that someone was using this driver in 2015, so
> I'm not sure anymore what to do. Of course we could still just remove
> it and later revert if someone steps up and claims the driver is still
> usable. Hmm. Does anyone any users of this driver?

What about moving the driver over into staging, which is generally the
way I understood to move a driver slowly out of the kernel?

Re: [PATCH] x86/pci: fix intel_mid_pci.c build error when ACPI is not enabled

[Jesse Barnes]

On Thu, Aug 13, 2020 at 12:58 PM Randy Dunlap  wrote:
>
> From: Randy Dunlap 
>
> Fix build error when CONFIG_ACPI is not set/enabled by adding
> the header file  which contains a stub for the function
> in the build error.
>
> ../arch/x86/pci/intel_mid_pci.c: In function ‘intel_mid_pci_init’:
> ../arch/x86/pci/intel_mid_pci.c:303:2: error: implicit declaration of 
> function ‘acpi_noirq_set’; did you mean ‘acpi_irq_get’? 
> [-Werror=implicit-function-declaration]
>   acpi_noirq_set();
>
> Signed-off-by: Randy Dunlap 
> Cc: Jacob Pan 
> Cc: Len Brown 
> Cc: Bjorn Helgaas 
> Cc: Jesse Barnes 
> Cc: Arjan van de Ven 
> Cc: linux-...@vger.kernel.org
> ---
> Found in linux-next, but applies to/exists in mainline also.
>
> Alternative.1: X86_INTEL_MID depends on ACPI
> Alternative.2: drop X86_INTEL_MID support
>
>  arch/x86/pci/intel_mid_pci.c |1 +
>  1 file changed, 1 insertion(+)
>
> --- linux-next-20200813.orig/arch/x86/pci/intel_mid_pci.c
> +++ linux-next-20200813/arch/x86/pci/intel_mid_pci.c
> @@ -33,6 +33,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>
>  #define PCIE_CAP_OFFSET    0x100

Reviewed-by: Jesse Barnes 

Thanks Randy. Another option is to remove the MID support entirely.

Jesse

Re: [PATCH 4.19 47/48] i40e: Memory leak in i40e_config_iwarp_qvlist

[Jesse Brandeburg]

On Tue, 11 Aug 2020 14:46:14 +0200
Pavel Machek  wrote:

> > --- a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
> > +++ b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
> > @@ -449,16 +450,19 @@ static int i40e_config_iwarp_qvlist(stru
> >  "Incorrect number of iwarp vectors %u.
> > Maximum %u allowed.\n", qvlist_info->num_vectors,
> >  msix_vf);
> > -   goto err;
> > +   ret = -EINVAL;
> > +   goto err_out;
> > }
> 
> And it is no longer freeing data qvlist_info() in this path. Is that
> correct? Should it goto err_free instead? 

Hi Pavel, thanks for the review.

I believe it is still correct, the logic is a bit convoluted, but
tracing back, I see that the caller in i40e_main.c allocates a buffer,
calls this function (eventually) with that memory cast to the *input*
variable qvlist_info, and then the top caller frees the original buffer.

One thing that I'll admit is confusing here is that the *input* struct
qvlist_info is different than the vf->qvlist_info struct managed by
this function. Maybe that they have the same name was confusing to you?
It confused me for a moment while I investigated, but I believe there
is no actual problem.

The reason for the function working this way is that the input data is
from the VF message, and the driver data structures in the PF (i40e)
driver representing state of the VF are managed separately. 

Jesse

Darlehensangebot

[Jesse Peterson]

Haben Sie nach Finanzierungsmöglichkeiten für Ihr Unternehmen, den Kauf eines 
neuen Eigenheims, den Bau, den Immobilienkredit, die Refinanzierung, die 
Schuldenkonsolidierung, den persönlichen oder geschäftlichen Zweck gesucht? 
Willkommen in der Zukunft! Finanzierung mit uns leicht gemacht. Kontaktieren 
Sie uns, da wir unseren Finanzdienstleister zu einem niedrigen und 
erschwinglichen Zinssatz von 2% für lange und kurze Darlehenslaufzeiten 
anbieten. Interessierte Antragsteller sollten sich unter 
jessepetersonl...@gmail.com an uns wenden, um weitere Verfahren zum Erwerb von 
Darlehen zu erhalten

Re: [RFC] Restrict the untrusted devices, to bind to only a set of "whitelisted" drivers

[Jesse Barnes]

> > I think your suggestion to disable driver binding once the initial
> > bus/slot devices have been bound will probably work for this
> > situation.  I just wanted to be clear that without some auditing,
> > fuzzing, and additional testing, we simply have to assume that drivers
> > are *not* secure and avoid using them on untrusted devices until we're
> > fairly confident they can handle them (whether just misbehaving or
> > malicious), in combination with other approaches like IOMMUs of
> > course.  And this isn't because we don't trust driver authors or
> > kernel developers to dtrt, it's just that for many devices (maybe USB
> > is an exception) I think driver authors haven't had to consider this
> > case much, and so I think it's prudent to expect bugs in this area
> > that we need to find & fix.
>
> For USB, yes, we have now had to deal with "untrusted devices" lieing
> about their ids and sending us horrible data.  That's all due to the
> fuzzing tools that have been written over the past few years, and now we
> have some of those in the kernel tree itself to help with that testing.
>
> For PCI, heh, good luck, those assumptions about "devices sending valid
> data" are everywhere, if our experience with USB is any indication.
>
> But, to take USB as an example, this is exactly what the USB
> "authorized" flag is there for, it's a "trust" setting that userspace
> has control over.  This came from the wireless USB spec, where it was
> determined that you could not trust devices.  So just use that same
> model here, move it to the driver core for all busses to use and you
> should be fine.
>
> If that doesn't meet your needs, please let me know the specifics of
> why, with patches :)

Yeah will do for sure.  I don't want to carry a big infra for this on our own!

> Now, as to you all getting some sort of "Hardware flag" to determine
> "inside" vs. "outside" devices, hah, good luck!  It took us a long time
> to get that for USB, and even then, BIOSes lie and get it wrong all the
> time.  So you will have to also deal with that in some way, for your
> userspace policy.

I think that's inherently platform specific to some extent.  We can do
it with our coreboot based firmware, but there's no guarantee other
vendors will adopt the same approach.  But I think at least for the
ChromeOS ecosystem we can come up with something that'll work, and
allow us to dtrt in userspace wrt driver binding.

Thanks,
Jesse

Re: [RFC] Restrict the untrusted devices, to bind to only a set of "whitelisted" drivers

[Jesse Barnes]

> > I feel a lot of resistance to the proposal, however, I'm not hearing
> > any realistic solutions that may help us to move forward. We want to
> > go with a solution that is acceptable upstream as that is our mission,
> > and also helps the community, however the behemoth task of "inspect
> > all drivers and fix them" before launching a product is really an
> > unfair ask I feel :-(. Can you help us by suggesting a proposal that
> > does not require us to trust a driver equally for internal / external
> > devices?
>
> I have no idea why you feel you have to "inspect all drivers" other than
> the fact that for some reason _you_ do not feel they are secure today.
>
> What type of "assurance" are you, or anyone else going to be able to
> provide for any kernel driver that would meet such a "I feel good now"
> level?  Have you done that work for any specific driver already so that
> you can show us what you mean by this effort?  Perhaps it's as simple as
> "oh look, this tool over here runs 'clean' on the source code, all is
> good!", or not, I really have no idea.

I think there's a disconnect somewhere in this discussion... maybe
we're just approaching this with different assumptions?

I think you recognize the potential for driver vulnerabilities when
binding to new or potentially hostile devices that may be spoofing
DID/VID/class/etc than then go on to abuse driver trust or the driver
using unvalidated inputs from the device to crash or run arbitrary
code on the target system.

Yes such drivers should be fixed, no doubt.  But without lots of
fuzzing (we're working on this) and testing we'd like to avoid
exposing that attack surface at all.

I think your suggestion to disable driver binding once the initial
bus/slot devices have been bound will probably work for this
situation.  I just wanted to be clear that without some auditing,
fuzzing, and additional testing, we simply have to assume that drivers
are *not* secure and avoid using them on untrusted devices until we're
fairly confident they can handle them (whether just misbehaving or
malicious), in combination with other approaches like IOMMUs of
course.  And this isn't because we don't trust driver authors or
kernel developers to dtrt, it's just that for many devices (maybe USB
is an exception) I think driver authors haven't had to consider this
case much, and so I think it's prudent to expect bugs in this area
that we need to find & fix.

Thanks,
Jesse

Re: [PATCH RFC] sched: Add a per-thread core scheduling interface

[Jesse Barnes]

On Thu, May 21, 2020 at 1:45 PM Joel Fernandes  wrote:
>
> Hi Linus,
>
> On Thu, May 21, 2020 at 11:31:38AM -0700, Linus Torvalds wrote:
> > On Wed, May 20, 2020 at 3:26 PM Joel Fernandes (Google)
> >  wrote:
> > Generally throughput benchmarks are much easier to do, how do you do
> > this latency benchmark, and is it perhaps something that could be run
> > more widely (ie I'm thinking that if it's generic enough and stable
> > enough to be run by some of the performance regression checking
> > robots, it would be a much more interesting test-case than some of the
> > ones they run right now...)
>
> Glad you like it! The metric is calculated with a timestamp of when the
> driver says the key was pressed, up until when the GPU says we've drawn
> pixels in response.
>
> The test requires a mostly only requires Chrome browser. It opens some
> pre-existing test URLs (a google doc, a window that opens a camera stream and
> another window that decodes video). This metric is already calculated in
> Chrome, we just scrape it from
> chrome://histograms/Event.Latency.EndToEnd.KeyPress.  If you install Chrome,
> you can goto this link and see the histogram.  We open a Google docs window
> and synthetically input keys into it with a camera stream and video decoding
> running in other windows which gives the CPUs a good beating. Then we collect
> roughly the 90th percentile keypress latency from the above histogram and the
> camera and decoded video's FPS, among other things. There is a test in the
> works that my colleagues are writing to run the full Google hangout video
> chatting stack to stress the system more (versus just the camera stream).  I
> guess if the robots can somehow input keys into the Google docs and open the
> right windows, then it is just a matter of scraping the histogram.

Expanding on this a little, we're working on a couple of projects that
should provide results like these for upstream.  One is continuously
rebasing our upstream backlog onto new kernels for testing purposes
(the idea here is to make it easier for us to update kernels on
Chromebooks), and the second is to drive more stuff into the
kernelci.org infrastructure.  Given the test environments we have in
place now, we can probably get results from our continuous rebase
project first and provide those against -rc releases if that's
something you'd be interested in.  Going forward, I hope we can
extract several of our tests and put them into kernelci as well, so we
get more general coverage without the potential impact of our (still
somewhat large) upstream backlog of patches.

To Joel's point, there are a few changes we'll have to make to get
similar results outside of our environment, but I think that's doable
without a ton of work.  And if anyone is curious, I think most of this
stuff is already public in the tast and autotest repos of the
chromiumos tree.  Just let us know if you want to make changes or port
to another environment so we can try to stay in sync wrt new features,
etc.

Thanks,
Jesse

Re: [PATCH v5] x86: bitops: fix build regression

[Jesse Brandeburg]

On Fri, 8 May 2020 13:28:35 -0700
Nathan Chancellor  wrote:

> On Fri, May 08, 2020 at 11:32:29AM -0700, Nick Desaulniers wrote:
> > Use the `%b` "x86 Operand Modifier" to instead force register allocation
> > to select a lower-8-bit GPR operand.

This looks OK to me, I appreciate the work done to find the right
fix and clean up the code while not breaking sparse! I had a look at
the assembly from gcc 9.3.1 and it looks good. Thanks!

Reviewed-by: Jesse Brandeburg 

Re: [PATCH v6 1/2] x86: fix bitops.h warning with a moved cast

[Jesse Brandeburg]

On Mon, 4 May 2020 12:51:12 -0700
Nick Desaulniers  wrote:

> Sorry for the very late report.  It turns out that if your config
> tickles __builtin_constant_p just right, this now produces invalid
> assembly:
> 
> $ cat foo.c
> long a(long b, long c) {
>   asm("orb\t%1, %0" : "+q"(c): "r"(b));
>   return c;
> }
> $ gcc foo.c
> foo.c: Assembler messages:
> foo.c:2: Error: `%rax' not allowed with `orb'
> 
> The "q" constraint only has meanting on -m32 otherwise is treated as
> "r".
> 
> Since we have the mask (& 0xff), can we drop the `b` suffix from the
> instruction? Or is a revert more appropriate? Or maybe another way to
> skin this cat?

Figures that such a small change can create problems :-( Sorry for the
thrash!

The patches in the link below basically add back the cast, but I'm
interested to see if any others can come up with a better fix that
a) passes the above code generation test
b) still keeps sparse happy
c) passes the test module and the code inspection

If need be I'm OK with a revert of the original patch to fix the issue
in the short term, but it seems to me there must be a way to satisfy
both tools.  We went through several iterations on the way to the final
patch that we might be able to pluck something useful from.

> Link: https://github.com/ClangBuiltLinux/linux/issues/961
> This is blowing up our KernelCI reports.

ASIDE: 
Bummer, how come none of those KernelCI reports are part of
zero-day testing at https://01.org/lkp/documentation/0-day-test-service
I'm interested in your answer but don't want to pollute this thread,
feel free to contact me directly for this one or start a new thread?

[PATCH 3/3] Staging: exfat: exfat_super.c Fixed coding style issues.

[Jesse Barton]

Removed function argument wrapping to new line.


Signed-off-by: Jesse Barton 
---
 drivers/staging/exfat/exfat_super.c | 29 +
 1 file changed, 9 insertions(+), 20 deletions(-)

diff --git a/drivers/staging/exfat/exfat_super.c 
b/drivers/staging/exfat/exfat_super.c
index 3c7e2b7c2195..b9656ec06144 100644
--- a/drivers/staging/exfat/exfat_super.c
+++ b/drivers/staging/exfat/exfat_super.c
@@ -640,8 +640,7 @@ static int ffs_lookup_file(struct inode *inode, char *path, 
struct file_id_t *fi
return ret;
 }
 
-static int ffs_create_file(struct inode *inode, char *path, u8 mode,
-struct file_id_t *fid)
+static int ffs_create_file(struct inode *inode, char *path, u8 mode, struct 
file_id_t *fid)
 {
struct chain_t dir;
struct uni_name_t uni_name;
@@ -681,8 +680,7 @@ static int ffs_create_file(struct inode *inode, char *path, 
u8 mode,
return ret;
 }
 
-static int ffs_read_file(struct inode *inode, struct file_id_t *fid, void 
*buffer,
-  u64 count, u64 *rcount)
+static int ffs_read_file(struct inode *inode, struct file_id_t *fid, void 
*buffer, u64 count, u64 *rcount)
 {
s32 offset, sec_offset, clu_offset;
u32 clu;
@@ -805,8 +803,7 @@ static int ffs_read_file(struct inode *inode, struct 
file_id_t *fid, void *buffe
return ret;
 }
 
-static int ffs_write_file(struct inode *inode, struct file_id_t *fid,
-   void *buffer, u64 count, u64 *wcount)
+static int ffs_write_file(struct inode *inode, struct file_id_t *fid, void 
*buffer, u64 count, u64 *wcount)
 {
bool modified = false;
s32 offset, sec_offset, clu_offset;
@@ -1212,8 +1209,7 @@ static void update_parent_info(struct file_id_t *fid,
}
 }
 
-static int ffs_move_file(struct inode *old_parent_inode, struct file_id_t *fid,
-  struct inode *new_parent_inode, struct dentry 
*new_dentry)
+static int ffs_move_file(struct inode *old_parent_inode, struct file_id_t 
*fid, struct inode *new_parent_inode, struct dentry *new_dentry)
 {
s32 ret;
s32 dentry;
@@ -2061,9 +2057,7 @@ static int ffs_read_dir(struct inode *inode, struct 
dir_entry_t *dir_entry)
fs_func->get_uni_name_from_ext_entry(sb, , dentry,
 uni_name.name);
if (*uni_name.name == 0x0 && p_fs->vol_type != EXFAT)
-   get_uni_name_from_dos_entry(sb,
-   (struct dos_dentry_t *)ep,
-   _name, 0x1);
+   get_uni_name_from_dos_entry(sb, (struct 
dos_dentry_t *)ep, _name, 0x1);
nls_uniname_to_cstring(sb, dir_entry->Name, _name);
buf_unlock(sb, sector);
 
@@ -2074,11 +2068,8 @@ static int ffs_read_dir(struct inode *inode, struct 
dir_entry_t *dir_entry)
goto out;
}
} else {
-   get_uni_name_from_dos_entry(sb,
-   (struct dos_dentry_t *)ep,
-   _name, 0x0);
-   nls_uniname_to_cstring(sb, dir_entry->ShortName,
-  _name);
+   get_uni_name_from_dos_entry(sb, (struct 
dos_dentry_t *)ep, _name, 0x0);
+   nls_uniname_to_cstring(sb, 
dir_entry->ShortName, _name);
}
 
dir_entry->Size = fs_func->get_entry_size(ep);
@@ -2460,8 +2451,7 @@ static struct dentry *exfat_lookup(struct inode *dir, 
struct dentry *dentry,
err = -ENOMEM;
goto error;
}
-   ffs_read_file(dir, , EXFAT_I(inode)->target,
-   i_size_read(inode), );
+   ffs_read_file(dir, , EXFAT_I(inode)->target, 
i_size_read(inode), );
*(EXFAT_I(inode)->target + i_size_read(inode)) = '\0';
}
 
@@ -2748,8 +2738,7 @@ static int exfat_rename(struct inode *old_dir, struct 
dentry *old_dentry,
 
EXFAT_I(old_inode)->fid.size = i_size_read(old_inode);
 
-   err = ffs_move_file(old_dir, &(EXFAT_I(old_inode)->fid), new_dir,
- new_dentry);
+   err = ffs_move_file(old_dir, &(EXFAT_I(old_inode)->fid), new_dir, 
new_dentry);
if (err) {
if (err == FFS_PERMISSIONERR)
err = -EPERM;
-- 
2.23.0

[PATCH 2/3] Staging: exfat: exfat_super.c: fixed multiple coding style issues with camelcase and parentheses

[Jesse Barton]

Changed Function Names:
ffsGetVolInfo -> ffs_get_vol_info
ffsLookupFile -> ffs_lookup_file
ffsCreateFile -> ffs_create_file
ffsReadFile -> ffs_read_file
ffsWriteFile -> ffs_write_file
ffsTruncateFile -> ffs_truncate_file
ffsRemoveFile -> ffs_remove_file
ffsMoveFile -> ffs_move_file
ffsSetAttr -> ffs_set_attr
ffsReadStat -> ffs_read_stat
ffsWriteStat -> ffs_write_stat
ffsMapCluster -> ffs_map_cluster

Removed BUG_ON() and added WARN_ON().
Removed unnecessary parentheses:
*(dir_entry->Name) - > *dir_entry->Name

Switched to lowercase o in these enums
Opt_uid
Opt_gid
Opt_umask
Opt_dmask
Opt_fmask
Opt_allow_utime
Opt_codepage
Opt_charset
Opt_namecase
Opt_debug
Opt_err_cont
Opt_err_panic
Opt_err_ro
Opt_utf8_hack
Opt_err

Signed-off-by: Jesse Barton 
---
 drivers/staging/exfat/exfat_super.c | 210 ++--
 1 file changed, 105 insertions(+), 105 deletions(-)

diff --git a/drivers/staging/exfat/exfat_super.c 
b/drivers/staging/exfat/exfat_super.c
index 665eb25e318d..3c7e2b7c2195 100644
--- a/drivers/staging/exfat/exfat_super.c
+++ b/drivers/staging/exfat/exfat_super.c
@@ -488,7 +488,7 @@ static int ffs_umount_vol(struct super_block *sb)
return err;
 }
 
-static int ffsGetVolInfo(struct super_block *sb, struct vol_info_t *info)
+static int ffs_get_vol_info(struct super_block *sb, struct vol_info_t *info)
 {
int err = FFS_SUCCESS;
struct fs_info_t *p_fs = &(EXFAT_SB(sb)->fs_info);
@@ -543,7 +543,7 @@ static int ffs_sync_vol(struct super_block *sb, bool 
do_sync)
 /*  File Operation Functions*/
 /*--*/
 
-static int ffsLookupFile(struct inode *inode, char *path, struct file_id_t 
*fid)
+static int ffs_lookup_file(struct inode *inode, char *path, struct file_id_t 
*fid)
 {
int ret, dentry, num_entries;
struct chain_t dir;
@@ -640,7 +640,7 @@ static int ffsLookupFile(struct inode *inode, char *path, 
struct file_id_t *fid)
return ret;
 }
 
-static int ffsCreateFile(struct inode *inode, char *path, u8 mode,
+static int ffs_create_file(struct inode *inode, char *path, u8 mode,
 struct file_id_t *fid)
 {
struct chain_t dir;
@@ -681,7 +681,7 @@ static int ffsCreateFile(struct inode *inode, char *path, 
u8 mode,
return ret;
 }
 
-static int ffsReadFile(struct inode *inode, struct file_id_t *fid, void 
*buffer,
+static int ffs_read_file(struct inode *inode, struct file_id_t *fid, void 
*buffer,
   u64 count, u64 *rcount)
 {
s32 offset, sec_offset, clu_offset;
@@ -805,7 +805,7 @@ static int ffsReadFile(struct inode *inode, struct 
file_id_t *fid, void *buffer,
return ret;
 }
 
-static int ffsWriteFile(struct inode *inode, struct file_id_t *fid,
+static int ffs_write_file(struct inode *inode, struct file_id_t *fid,
void *buffer, u64 count, u64 *wcount)
 {
bool modified = false;
@@ -1002,13 +1002,13 @@ static int ffsWriteFile(struct inode *inode, struct 
file_id_t *fid,
 
/* (3) update the direcoty entry */
if (p_fs->vol_type == EXFAT) {
-   es = get_entry_set_in_dir(sb, &(fid->dir), fid->entry,
+   es = get_entry_set_in_dir(sb, >dir, fid->entry,
  ES_ALL_ENTRIES, );
if (!es)
goto err_out;
ep2 = ep + 1;
} else {
-   ep = get_entry_in_dir(sb, &(fid->dir), fid->entry, );
+   ep = get_entry_in_dir(sb, >dir, fid->entry, );
if (!ep)
goto err_out;
ep2 = ep;
@@ -1062,7 +1062,7 @@ static int ffsWriteFile(struct inode *inode, struct 
file_id_t *fid,
return ret;
 }
 
-static int ffsTruncateFile(struct inode *inode, u64 old_size, u64 new_size)
+static int ffs_truncate_file(struct inode *inode, u64 old_size, u64 new_size)
 {
s32 num_clusters;
u32 last_clu = CLUSTER_32(0);
@@ -1141,7 +1141,7 @@ static int ffsTruncateFile(struct inode *inode, u64 
old_size, u64 new_size)
}
ep2 = ep + 1;
} else {
-   ep = get_entry_in_dir(sb, &(fid->dir), fid->entry, );
+   ep = get_entry_in_dir(sb, >dir, fid->entry, );
if (!ep) {
ret = FFS_MEDIAERR;
goto out;
@@ -1212,7 +1212,7 @@ static void update_parent_info(struct file_id_t *fid,
}
 }
 
-static int ffsMoveFile(struct inode *old_parent_inode, struct file_id_t *fid,
+static int ffs_move_file(struct inode *old_parent_inode, struct file_id_t *fid,
   struct inode *new_parent_inode, struct dentry 
*new_dentry)
 {
s32 ret;
@@ -1276,7 +1276,7 @@ static int ffsMoveFile(struct inode *old_parent_inode, 
struct f

[PATCH 1/3] Staging: exfat: exfat_super.c: fixed camelcase coding style issue

[Jesse Barton]

Changed function names:
ffsUmountVol to ffs_umount_vol
ffsMountVol to ffs_mount_vol
ffsSyncVol to ffs_sync_vol


Signed-off-by: Jesse Barton 
---
 drivers/staging/exfat/exfat_super.c | 20 ++--
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/staging/exfat/exfat_super.c 
b/drivers/staging/exfat/exfat_super.c
index 5f6caee819a6..665eb25e318d 100644
--- a/drivers/staging/exfat/exfat_super.c
+++ b/drivers/staging/exfat/exfat_super.c
@@ -342,7 +342,7 @@ static inline void exfat_save_attr(struct inode *inode, u32 
attr)
EXFAT_I(inode)->fid.attr = attr & (ATTR_RWMASK | ATTR_READONLY);
 }
 
-static int ffsMountVol(struct super_block *sb)
+static int ffs_mount_vol(struct super_block *sb)
 {
int i, ret;
struct pbr_sector_t *p_pbr;
@@ -446,7 +446,7 @@ static int ffsMountVol(struct super_block *sb)
return ret;
 }
 
-static int ffsUmountVol(struct super_block *sb)
+static int ffs_umount_vol(struct super_block *sb)
 {
struct fs_info_t *p_fs = &(EXFAT_SB(sb)->fs_info);
int err = FFS_SUCCESS;
@@ -518,7 +518,7 @@ static int ffsGetVolInfo(struct super_block *sb, struct 
vol_info_t *info)
return err;
 }
 
-static int ffsSyncVol(struct super_block *sb, bool do_sync)
+static int ffs_sync_vol(struct super_block *sb, bool do_sync)
 {
int err = FFS_SUCCESS;
struct fs_info_t *p_fs = &(EXFAT_SB(sb)->fs_info);
@@ -3043,7 +3043,7 @@ static int exfat_file_release(struct inode *inode, struct 
file *filp)
struct super_block *sb = inode->i_sb;
 
EXFAT_I(inode)->fid.size = i_size_read(inode);
-   ffsSyncVol(sb, false);
+   ffs_sync_vol(sb, false);
return 0;
 }
 
@@ -3460,7 +3460,7 @@ static void exfat_put_super(struct super_block *sb)
if (__is_sb_dirty(sb))
exfat_write_super(sb);
 
-   ffsUmountVol(sb);
+   ffs_umount_vol(sb);
 
sb->s_fs_info = NULL;
exfat_free_super(sbi);
@@ -3473,7 +3473,7 @@ static void exfat_write_super(struct super_block *sb)
__set_sb_clean(sb);
 
if (!sb_rdonly(sb))
-   ffsSyncVol(sb, true);
+   ffs_sync_vol(sb, true);
 
__unlock_super(sb);
 }
@@ -3485,7 +3485,7 @@ static int exfat_sync_fs(struct super_block *sb, int wait)
if (__is_sb_dirty(sb)) {
__lock_super(sb);
__set_sb_clean(sb);
-   err = ffsSyncVol(sb, true);
+   err = ffs_sync_vol(sb, true);
__unlock_super(sb);
}
 
@@ -3865,10 +3865,10 @@ static int exfat_fill_super(struct super_block *sb, 
void *data, int silent)
sb_min_blocksize(sb, 512);
sb->s_maxbytes = 0x7fffLL;/* maximum file size */
 
-   ret = ffsMountVol(sb);
+   ret = ffs_mount_vol(sb);
if (ret) {
if (!silent)
-   pr_err("[EXFAT] ffsMountVol failed\n");
+   pr_err("[EXFAT] ffs_mount_vol failed\n");
 
goto out_fail;
}
@@ -3919,7 +3919,7 @@ static int exfat_fill_super(struct super_block *sb, void 
*data, int silent)
return 0;
 
 out_fail2:
-   ffsUmountVol(sb);
+   ffs_umount_vol(sb);
 out_fail:
if (root_inode)
iput(root_inode);
-- 
2.23.0

[PATCH 2/3] Staging: exfat: exfat_super.c: fixed multiple coding style issues with camelcase and parentheses

[Jesse Barton]

Fixed coding style issues with camelcase on functions and various parentheses 
that were not needed

Signed-off-by: Jesse Barton 
---
 drivers/staging/exfat/exfat_super.c | 210 ++--
 1 file changed, 105 insertions(+), 105 deletions(-)

diff --git a/drivers/staging/exfat/exfat_super.c 
b/drivers/staging/exfat/exfat_super.c
index 665eb25e318d..3c7e2b7c2195 100644
--- a/drivers/staging/exfat/exfat_super.c
+++ b/drivers/staging/exfat/exfat_super.c
@@ -488,7 +488,7 @@ static int ffs_umount_vol(struct super_block *sb)
return err;
 }
 
-static int ffsGetVolInfo(struct super_block *sb, struct vol_info_t *info)
+static int ffs_get_vol_info(struct super_block *sb, struct vol_info_t *info)
 {
int err = FFS_SUCCESS;
struct fs_info_t *p_fs = &(EXFAT_SB(sb)->fs_info);
@@ -543,7 +543,7 @@ static int ffs_sync_vol(struct super_block *sb, bool 
do_sync)
 /*  File Operation Functions*/
 /*--*/
 
-static int ffsLookupFile(struct inode *inode, char *path, struct file_id_t 
*fid)
+static int ffs_lookup_file(struct inode *inode, char *path, struct file_id_t 
*fid)
 {
int ret, dentry, num_entries;
struct chain_t dir;
@@ -640,7 +640,7 @@ static int ffsLookupFile(struct inode *inode, char *path, 
struct file_id_t *fid)
return ret;
 }
 
-static int ffsCreateFile(struct inode *inode, char *path, u8 mode,
+static int ffs_create_file(struct inode *inode, char *path, u8 mode,
 struct file_id_t *fid)
 {
struct chain_t dir;
@@ -681,7 +681,7 @@ static int ffsCreateFile(struct inode *inode, char *path, 
u8 mode,
return ret;
 }
 
-static int ffsReadFile(struct inode *inode, struct file_id_t *fid, void 
*buffer,
+static int ffs_read_file(struct inode *inode, struct file_id_t *fid, void 
*buffer,
   u64 count, u64 *rcount)
 {
s32 offset, sec_offset, clu_offset;
@@ -805,7 +805,7 @@ static int ffsReadFile(struct inode *inode, struct 
file_id_t *fid, void *buffer,
return ret;
 }
 
-static int ffsWriteFile(struct inode *inode, struct file_id_t *fid,
+static int ffs_write_file(struct inode *inode, struct file_id_t *fid,
void *buffer, u64 count, u64 *wcount)
 {
bool modified = false;
@@ -1002,13 +1002,13 @@ static int ffsWriteFile(struct inode *inode, struct 
file_id_t *fid,
 
/* (3) update the direcoty entry */
if (p_fs->vol_type == EXFAT) {
-   es = get_entry_set_in_dir(sb, &(fid->dir), fid->entry,
+   es = get_entry_set_in_dir(sb, >dir, fid->entry,
  ES_ALL_ENTRIES, );
if (!es)
goto err_out;
ep2 = ep + 1;
} else {
-   ep = get_entry_in_dir(sb, &(fid->dir), fid->entry, );
+   ep = get_entry_in_dir(sb, >dir, fid->entry, );
if (!ep)
goto err_out;
ep2 = ep;
@@ -1062,7 +1062,7 @@ static int ffsWriteFile(struct inode *inode, struct 
file_id_t *fid,
return ret;
 }
 
-static int ffsTruncateFile(struct inode *inode, u64 old_size, u64 new_size)
+static int ffs_truncate_file(struct inode *inode, u64 old_size, u64 new_size)
 {
s32 num_clusters;
u32 last_clu = CLUSTER_32(0);
@@ -1141,7 +1141,7 @@ static int ffsTruncateFile(struct inode *inode, u64 
old_size, u64 new_size)
}
ep2 = ep + 1;
} else {
-   ep = get_entry_in_dir(sb, &(fid->dir), fid->entry, );
+   ep = get_entry_in_dir(sb, >dir, fid->entry, );
if (!ep) {
ret = FFS_MEDIAERR;
goto out;
@@ -1212,7 +1212,7 @@ static void update_parent_info(struct file_id_t *fid,
}
 }
 
-static int ffsMoveFile(struct inode *old_parent_inode, struct file_id_t *fid,
+static int ffs_move_file(struct inode *old_parent_inode, struct file_id_t *fid,
   struct inode *new_parent_inode, struct dentry 
*new_dentry)
 {
s32 ret;
@@ -1276,7 +1276,7 @@ static int ffsMoveFile(struct inode *old_parent_inode, 
struct file_id_t *fid,
 
update_parent_info(new_fid, new_parent_inode);
 
-   p_dir = &(new_fid->dir);
+   p_dir = _fid->dir;
new_entry = new_fid->entry;
ep = get_entry_in_dir(sb, p_dir, new_entry, NULL);
if (!ep)
@@ -1341,7 +1341,7 @@ static int ffsMoveFile(struct inode *old_parent_inode, 
struct file_id_t *fid,
return ret;
 }
 
-static int ffsRemoveFile(struct inode *inode, struct file_id_t *fid)
+static int ffs_remove_file(struct inode *inode, struct file_id_t *fid)
 {
s32 dentry;
int ret = FFS_SUCCESS;
@@ -1405,7 +1405,7 @@ static int ffsRemoveFile(

[PATCH 3/3] Staging: exfat: exfat_super.c Fixed coding style issues.

[Jesse Barton]

Fixed Coding Style issues

Signed-off-by: Jesse Barton 
---
 drivers/staging/exfat/exfat_super.c | 29 +
 1 file changed, 9 insertions(+), 20 deletions(-)

diff --git a/drivers/staging/exfat/exfat_super.c 
b/drivers/staging/exfat/exfat_super.c
index 3c7e2b7c2195..b9656ec06144 100644
--- a/drivers/staging/exfat/exfat_super.c
+++ b/drivers/staging/exfat/exfat_super.c
@@ -640,8 +640,7 @@ static int ffs_lookup_file(struct inode *inode, char *path, 
struct file_id_t *fi
return ret;
 }
 
-static int ffs_create_file(struct inode *inode, char *path, u8 mode,
-struct file_id_t *fid)
+static int ffs_create_file(struct inode *inode, char *path, u8 mode, struct 
file_id_t *fid)
 {
struct chain_t dir;
struct uni_name_t uni_name;
@@ -681,8 +680,7 @@ static int ffs_create_file(struct inode *inode, char *path, 
u8 mode,
return ret;
 }
 
-static int ffs_read_file(struct inode *inode, struct file_id_t *fid, void 
*buffer,
-  u64 count, u64 *rcount)
+static int ffs_read_file(struct inode *inode, struct file_id_t *fid, void 
*buffer, u64 count, u64 *rcount)
 {
s32 offset, sec_offset, clu_offset;
u32 clu;
@@ -805,8 +803,7 @@ static int ffs_read_file(struct inode *inode, struct 
file_id_t *fid, void *buffe
return ret;
 }
 
-static int ffs_write_file(struct inode *inode, struct file_id_t *fid,
-   void *buffer, u64 count, u64 *wcount)
+static int ffs_write_file(struct inode *inode, struct file_id_t *fid, void 
*buffer, u64 count, u64 *wcount)
 {
bool modified = false;
s32 offset, sec_offset, clu_offset;
@@ -1212,8 +1209,7 @@ static void update_parent_info(struct file_id_t *fid,
}
 }
 
-static int ffs_move_file(struct inode *old_parent_inode, struct file_id_t *fid,
-  struct inode *new_parent_inode, struct dentry 
*new_dentry)
+static int ffs_move_file(struct inode *old_parent_inode, struct file_id_t 
*fid, struct inode *new_parent_inode, struct dentry *new_dentry)
 {
s32 ret;
s32 dentry;
@@ -2061,9 +2057,7 @@ static int ffs_read_dir(struct inode *inode, struct 
dir_entry_t *dir_entry)
fs_func->get_uni_name_from_ext_entry(sb, , dentry,
 uni_name.name);
if (*uni_name.name == 0x0 && p_fs->vol_type != EXFAT)
-   get_uni_name_from_dos_entry(sb,
-   (struct dos_dentry_t *)ep,
-   _name, 0x1);
+   get_uni_name_from_dos_entry(sb, (struct 
dos_dentry_t *)ep, _name, 0x1);
nls_uniname_to_cstring(sb, dir_entry->Name, _name);
buf_unlock(sb, sector);
 
@@ -2074,11 +2068,8 @@ static int ffs_read_dir(struct inode *inode, struct 
dir_entry_t *dir_entry)
goto out;
}
} else {
-   get_uni_name_from_dos_entry(sb,
-   (struct dos_dentry_t *)ep,
-   _name, 0x0);
-   nls_uniname_to_cstring(sb, dir_entry->ShortName,
-  _name);
+   get_uni_name_from_dos_entry(sb, (struct 
dos_dentry_t *)ep, _name, 0x0);
+   nls_uniname_to_cstring(sb, 
dir_entry->ShortName, _name);
}
 
dir_entry->Size = fs_func->get_entry_size(ep);
@@ -2460,8 +2451,7 @@ static struct dentry *exfat_lookup(struct inode *dir, 
struct dentry *dentry,
err = -ENOMEM;
goto error;
}
-   ffs_read_file(dir, , EXFAT_I(inode)->target,
-   i_size_read(inode), );
+   ffs_read_file(dir, , EXFAT_I(inode)->target, 
i_size_read(inode), );
*(EXFAT_I(inode)->target + i_size_read(inode)) = '\0';
}
 
@@ -2748,8 +2738,7 @@ static int exfat_rename(struct inode *old_dir, struct 
dentry *old_dentry,
 
EXFAT_I(old_inode)->fid.size = i_size_read(old_inode);
 
-   err = ffs_move_file(old_dir, &(EXFAT_I(old_inode)->fid), new_dir,
- new_dentry);
+   err = ffs_move_file(old_dir, &(EXFAT_I(old_inode)->fid), new_dir, 
new_dentry);
if (err) {
if (err == FFS_PERMISSIONERR)
err = -EPERM;
-- 
2.23.0

[PATCH] Staging: exfat: exfat_super.c: fixed camelcase coding style issue

[Jesse Barton]

Fixed a coding style issue.

Signed-off-by: Jesse Barton 
---
 drivers/staging/exfat/exfat_super.c | 20 ++--
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/staging/exfat/exfat_super.c 
b/drivers/staging/exfat/exfat_super.c
index 5f6caee819a6..665eb25e318d 100644
--- a/drivers/staging/exfat/exfat_super.c
+++ b/drivers/staging/exfat/exfat_super.c
@@ -342,7 +342,7 @@ static inline void exfat_save_attr(struct inode *inode, u32 
attr)
EXFAT_I(inode)->fid.attr = attr & (ATTR_RWMASK | ATTR_READONLY);
 }
 
-static int ffsMountVol(struct super_block *sb)
+static int ffs_mount_vol(struct super_block *sb)
 {
int i, ret;
struct pbr_sector_t *p_pbr;
@@ -446,7 +446,7 @@ static int ffsMountVol(struct super_block *sb)
return ret;
 }
 
-static int ffsUmountVol(struct super_block *sb)
+static int ffs_umount_vol(struct super_block *sb)
 {
struct fs_info_t *p_fs = &(EXFAT_SB(sb)->fs_info);
int err = FFS_SUCCESS;
@@ -518,7 +518,7 @@ static int ffsGetVolInfo(struct super_block *sb, struct 
vol_info_t *info)
return err;
 }
 
-static int ffsSyncVol(struct super_block *sb, bool do_sync)
+static int ffs_sync_vol(struct super_block *sb, bool do_sync)
 {
int err = FFS_SUCCESS;
struct fs_info_t *p_fs = &(EXFAT_SB(sb)->fs_info);
@@ -3043,7 +3043,7 @@ static int exfat_file_release(struct inode *inode, struct 
file *filp)
struct super_block *sb = inode->i_sb;
 
EXFAT_I(inode)->fid.size = i_size_read(inode);
-   ffsSyncVol(sb, false);
+   ffs_sync_vol(sb, false);
return 0;
 }
 
@@ -3460,7 +3460,7 @@ static void exfat_put_super(struct super_block *sb)
if (__is_sb_dirty(sb))
exfat_write_super(sb);
 
-   ffsUmountVol(sb);
+   ffs_umount_vol(sb);
 
sb->s_fs_info = NULL;
exfat_free_super(sbi);
@@ -3473,7 +3473,7 @@ static void exfat_write_super(struct super_block *sb)
__set_sb_clean(sb);
 
if (!sb_rdonly(sb))
-   ffsSyncVol(sb, true);
+   ffs_sync_vol(sb, true);
 
__unlock_super(sb);
 }
@@ -3485,7 +3485,7 @@ static int exfat_sync_fs(struct super_block *sb, int wait)
if (__is_sb_dirty(sb)) {
__lock_super(sb);
__set_sb_clean(sb);
-   err = ffsSyncVol(sb, true);
+   err = ffs_sync_vol(sb, true);
__unlock_super(sb);
}
 
@@ -3865,10 +3865,10 @@ static int exfat_fill_super(struct super_block *sb, 
void *data, int silent)
sb_min_blocksize(sb, 512);
sb->s_maxbytes = 0x7fffLL;/* maximum file size */
 
-   ret = ffsMountVol(sb);
+   ret = ffs_mount_vol(sb);
if (ret) {
if (!silent)
-   pr_err("[EXFAT] ffsMountVol failed\n");
+   pr_err("[EXFAT] ffs_mount_vol failed\n");
 
goto out_fail;
}
@@ -3919,7 +3919,7 @@ static int exfat_fill_super(struct super_block *sb, void 
*data, int silent)
return 0;
 
 out_fail2:
-   ffsUmountVol(sb);
+   ffs_umount_vol(sb);
 out_fail:
if (root_inode)
iput(root_inode);
-- 
2.23.0

Re: [PATCH net-next 0/6] net: stmmac: Improvements for -next

[Jesse Brandeburg]

On Tue, 10 Sep 2019 16:41:21 +0200 Jose wrote:
> Misc patches for -next. It includes:
>  - Two fixes for features in -next only
>  - New features support for GMAC cores (which includes GMAC4 and GMAC5)
> 
> ---
> Cc: Giuseppe Cavallaro 
> Cc: Alexandre Torgue 
> Cc: Jose Abreu 
> Cc: "David S. Miller" 
> Cc: Maxime Coquelin 
> Cc: net...@vger.kernel.org
> Cc: linux-st...@st-md-mailman.stormreply.com
> Cc: linux-arm-ker...@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org
> ---
> 
> Jose Abreu (6):
>   net: stmmac: Prevent divide-by-zero
>   net: stmmac: Add VLAN HASH filtering support in GMAC4+
>   net: stmmac: xgmac: Reinitialize correctly a variable
>   net: stmmac: Add support for SA Insertion/Replacement in GMAC4+
>   net: stmmac: Add support for VLAN Insertion Offload in GMAC4+
>   net: stmmac: ARP Offload for GMAC4+ Cores

For the series, looks good to me.

Reviewed-by: Jesse Brandeburg 

Re: [PATCH] team: Add vlan tx offload to hw_enc_features

[Jesse Brandeburg]

On Wed, 7 Aug 2019 10:38:08 +0800
YueHaibing  wrote:

> We should also enable bonding's vlan tx offload in hw_enc_features,

You mean team's vlan tx offload?

> pass the vlan packets to the slave devices with vlan tci, let them

s/let them to/let the slave/

> to handle vlan tunneling offload implementation.
> 
> Fixes: 3268e5cb494d ("team: Advertise tunneling offload features")
> Signed-off-by: YueHaibing 

Re: [PATCH] myri10ge: remove unneeded variable

[Jesse Brandeburg]

On Wed, 31 Jul 2019 16:53:46 +0800
Ding Xiang  wrote:

> "error" is unneeded,just return 0
> 
> Signed-off-by: Ding Xiang 

Reviewed-by: Jesse Brandeburg 

Re: [PATCH 2/2] net: ag71xx: Use GFP_KERNEL instead of GFP_ATOMIC in 'ag71xx_rings_init()'

[Jesse Brandeburg]

On Wed, 31 Jul 2019 10:06:48 +0200
Christophe JAILLET  wrote:

> There is no need to use GFP_ATOMIC here, GFP_KERNEL should be enough.
> The 'kcalloc()' just a few lines above, already uses GFP_KERNEL.
> 
> Signed-off-by: Christophe JAILLET 

Reviewed-by: Jesse Brandeburg 

Re: [PATCH 1/2] net: ag71xx: Slighly simplify code in 'ag71xx_rings_init()'

[Jesse Brandeburg]

On Wed, 31 Jul 2019 10:06:38 +0200
Christophe JAILLET  wrote:

> A few lines above, we have:
>tx_size = BIT(tx->order);
> 
> So use 'tx_size' directly to be consistent with the way 'rx->descs_cpu' and
> 'rx->descs_dma' are computed below.
> 
> Signed-off-by: Christophe JAILLET 

Reviewed-by: Jesse Brandeburg 

Re: [PATCH] net: ethernet: et131x: Use GFP_KERNEL instead of GFP_ATOMIC when allocating tx_ring->tcb_ring

[Jesse Brandeburg]

On Wed, 31 Jul 2019 09:38:42 +0200
Christophe JAILLET  wrote:

> There is no good reason to use GFP_ATOMIC here. Other memory allocations
> are performed with GFP_KERNEL (see other 'dma_alloc_coherent()' below and
> 'kzalloc()' in 'et131x_rx_dma_memory_alloc()')
> 
> Use GFP_KERNEL which should be enough.
> 
> Signed-off-by: Christophe JAILLET 

Sure, but generally I'd say GFP_ATOMIC is ok if you're in an init path
and you can afford to have the allocation thread sleep while memory is
being found by the kernel.

Reviewed-by: Jesse Brandeburg 

Re: [PATCH net-next] net: Drop unlikely before IS_ERR(_OR_NULL)

[Jesse Brandeburg]

On Wed, 5 Jun 2019 22:24:26 +0800 Kefeng wrote:
> IS_ERR(_OR_NULL) already contain an 'unlikely' compiler flag,
> so no need to do that again from its callers. Drop it.
> 



>   segs = __skb_gso_segment(skb, features, false);
> - if (unlikely(IS_ERR_OR_NULL(segs))) {
> + if (IS_ERR_OR_NULL(segs)) {
>   int segs_nr = skb_shinfo(skb)->gso_segs;
>  

The change itself seems reasonable, but did you check to see if the
paths changed are faster/slower with your fix?  Did you look at any
assembly output to see if the compiler actually generated different
code?  Is there a set of similar changes somewhere else in the kernel
we can refer to?

I'm not sure in the end that the change is worth it, so would like you
to prove it is, unless davem overrides me. :-)

Re: [PATCH net-next v4] hinic: add LRO support

[Jesse Brandeburg]

On Tue, 4 Jun 2019 01:16:08 + Xue wrote:
> This patch adds LRO support for the HiNIC driver.
> 
> Reported-by: kbuild test robot 
> Reviewed-by: Jesse Brandeburg 
> Signed-off-by: Xue Chaojing 

Hm, you added my reviewed-by tag, but I didn't add it myself, I
only commented on your code.  This is a no-no. You don't add tags with
other people's names just because you think you can/should.

If someone EXPLICITLY says: "go ahead and add my reviewed-by after these
changes" then you can add it yourself.

Also, what did you change in v1:v4? There should be a summary somewhere
in the commit log (usually after a ---)

FWIW, I looked over the code and my concerns were addressed.

Reviewed-by: Jesse Brandeburg 

Re: [PATCH net-next v3] hinic: add LRO support

[Jesse Brandeburg]

some review comments below...

On Mon, 3 Jun 2019 04:35:36 +
Xue Chaojing  wrote:

> This patch adds LRO support for the HiNIC driver.
> 
> Signed-off-by: Xue Chaojing 
> ---
>  .../net/ethernet/huawei/hinic/hinic_hw_dev.c  |   2 +
>  .../net/ethernet/huawei/hinic/hinic_hw_dev.h  |   8 +-
>  .../net/ethernet/huawei/hinic/hinic_hw_io.c   |  58 +
>  .../ethernet/huawei/hinic/hinic_hw_qp_ctxt.h  |   5 +
>  .../net/ethernet/huawei/hinic/hinic_hw_wqe.h  |  22 +++-
>  .../net/ethernet/huawei/hinic/hinic_main.c|  51 +++-
>  .../net/ethernet/huawei/hinic/hinic_port.c| 114 ++
>  .../net/ethernet/huawei/hinic/hinic_port.h|  45 +++
>  drivers/net/ethernet/huawei/hinic/hinic_rx.c  |  46 +--
>  drivers/net/ethernet/huawei/hinic/hinic_rx.h  |   2 +
>  10 files changed, 340 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/net/ethernet/huawei/hinic/hinic_hw_dev.c 
> b/drivers/net/ethernet/huawei/hinic/hinic_hw_dev.c
> index 3875f39f43bb..756a7e3280bd 100644
> --- a/drivers/net/ethernet/huawei/hinic/hinic_hw_dev.c
> +++ b/drivers/net/ethernet/huawei/hinic/hinic_hw_dev.c
> @@ -313,6 +313,8 @@ static int set_hw_ioctxt(struct hinic_hwdev *hwdev, 
> unsigned int rq_depth,
>   hw_ioctxt.set_cmdq_depth = HW_IOCTXT_SET_CMDQ_DEPTH_DEFAULT;
>   hw_ioctxt.cmdq_depth = 0;
>  
> + hw_ioctxt.lro_en = 1;
> +
>   hw_ioctxt.rq_depth  = ilog2(rq_depth);
>  
>   hw_ioctxt.rx_buf_sz_idx = HINIC_RX_BUF_SZ_IDX;
> diff --git a/drivers/net/ethernet/huawei/hinic/hinic_hw_dev.h 
> b/drivers/net/ethernet/huawei/hinic/hinic_hw_dev.h
> index c9e621e19dd0..fba4fe82472a 100644
> --- a/drivers/net/ethernet/huawei/hinic/hinic_hw_dev.h
> +++ b/drivers/net/ethernet/huawei/hinic/hinic_hw_dev.h
> @@ -50,6 +50,8 @@ enum hinic_port_cmd {
>  
>   HINIC_PORT_CMD_GET_LINK_STATE   = 24,
>  
> + HINIC_PORT_CMD_SET_LRO  = 25,
> +
>   HINIC_PORT_CMD_SET_RX_CSUM  = 26,
>  
>   HINIC_PORT_CMD_SET_PORT_STATE   = 41,
> @@ -62,7 +64,11 @@ enum hinic_port_cmd {
>  
>   HINIC_PORT_CMD_SET_TSO  = 112,
>  
> + HINIC_PORT_CMD_SET_RQ_IQ_MAP= 115,
> +
>   HINIC_PORT_CMD_GET_CAP  = 170,
> +
> + HINIC_PORT_CMD_SET_LRO_TIMER= 244,
>  };
>  
>  enum hinic_mgmt_msg_cmd {
> @@ -106,7 +112,7 @@ struct hinic_cmd_hw_ioctxt {
>   u8  set_cmdq_depth;
>   u8  cmdq_depth;
>  
> - u8  rsvd2;
> + u8  lro_en;
>   u8  rsvd3;
>   u8  rsvd4;
>   u8  rsvd5;
> diff --git a/drivers/net/ethernet/huawei/hinic/hinic_hw_io.c 
> b/drivers/net/ethernet/huawei/hinic/hinic_hw_io.c
> index a322a22d9357..1169526323cf 100644
> --- a/drivers/net/ethernet/huawei/hinic/hinic_hw_io.c
> +++ b/drivers/net/ethernet/huawei/hinic/hinic_hw_io.c
> @@ -45,6 +45,7 @@
>  
>  enum io_cmd {
>   IO_CMD_MODIFY_QUEUE_CTXT = 0,
> + IO_CMD_CLEAN_QUEUE_CTXT,
>  };
>  
>  static void init_db_area_idx(struct hinic_free_db_area *free_db_area)
> @@ -210,6 +211,57 @@ static int write_qp_ctxts(struct hinic_func_to_io 
> *func_to_io, u16 base_qpn,
>   write_rq_ctxts(func_to_io, base_qpn, num_qps));
>  }
>  
> +static int clean_queue_offload_ctxt(struct hinic_func_to_io *func_to_io,
> + enum hinic_qp_ctxt_type ctxt_type)

all the other functions you add have hinic_ in front of them, how come
not this one?

> +{
> + struct hinic_hwif *hwif = func_to_io->hwif;
> + struct hinic_clean_queue_ctxt *ctxt_block;
> + struct pci_dev *pdev = hwif->pdev;
> + struct hinic_cmdq_buf cmdq_buf;
> + u64 out_param = 0;
> + int err;
> +
> + err = hinic_alloc_cmdq_buf(_to_io->cmdqs, _buf);
> + if (err) {
> + dev_err(>dev, "Failed to allocate cmdq buf\n");
> + return err;
> + }
> +
> + ctxt_block = cmdq_buf.buf;
> + ctxt_block->cmdq_hdr.num_queues = func_to_io->max_qps;
> + ctxt_block->cmdq_hdr.queue_type = ctxt_type;
> + ctxt_block->cmdq_hdr.addr_offset = 0;
> +
> + /* TSO/LRO ctxt size: 0x0:0B; 0x1:160B; 0x2:200B; 0x3:240B */
> + ctxt_block->ctxt_size = 0x3;
> +
> + hinic_cpu_to_be32(ctxt_block, sizeof(*ctxt_block));
> +
> + cmdq_buf.size = sizeof(*ctxt_block);
> +
> + err = hinic_cmdq_direct_resp(_to_io->cmdqs, HINIC_MOD_L2NIC,
> +  IO_CMD_CLEAN_QUEUE_CTXT,
> +  _buf, _param);
> +
> + if (err || out_param) {
> + dev_err(>dev, "Failed to clean queue offload ctxts, err: 
> %d, out_param: 0x%llx\n",
> + err, out_param);
> +
> + err = -EFAULT;
> + }
> +
> + hinic_free_cmdq_buf(_to_io->cmdqs, _buf);
> +
> + return err;
> +}
> +
> +static int clean_qp_offload_ctxt(struct hinic_func_to_io *func_to_io)
> +{
> + /* clean LRO/TSO context space */
> + return (clean_queue_offload_ctxt(func_to_io, HINIC_QP_CTXT_TYPE_SQ) ||
> + clean_queue_offload_ctxt(func_to_io, 

Re: Regression causes a hang on boot with a Comtrol PCI card

[Jesse Hathaway]

On Fri, Mar 22, 2019 at 3:02 PM Jesse Hathaway  wrote:
> > Can you boot v5.0 vanilla with "initcall_debug"?  Maybe we can narrow
> > it down to a specific quirk.
>
> yup, added the "initcall_debug" output to the ticket:
> https://bugzilla.kernel.org/show_bug.cgi?id=202927, here is the tail end
>
> [   14.896337] NET: Registered protocol family 1
> [   14.901314] initcall af_unix_init+0x0/0x4e returned 0 after 4866 usecs
> [   14.908694] calling  ipv6_offload_init+0x0/0x7f @ 1
> [   14.914238] initcall ipv6_offload_init+0x0/0x7f returned 0 after 1 usecs
> [   14.921821] calling  vlan_offload_init+0x0/0x20 @ 1
> [   14.927365] initcall vlan_offload_init+0x0/0x20 returned 0 after 0 usecs
> [   14.934948] calling  pci_apply_final_quirks+0x0/0x126 @ 1
> [   14.941106] pci :00:1a.0: calling  quirk_usb_early_handoff+0x0/0x6a0 @ 
> 1

Bjorn, did you get a chance to look at the initcall_debug output for anything
obvious to you on what might be the cause of the problem?

Thanks, Jesse Hathaway

Re: Regression causes a hang on boot with a Comtrol PCI card

[Jesse Hathaway]

> So apparently the hang happens while we're running the "final" PCI
> fixups.  This happens after all the rest of PCI is initialized.
>
> Can you boot v5.0 vanilla with "initcall_debug"?  Maybe we can narrow
> it down to a specific quirk.

yup, added the "initcall_debug" output to the ticket:
https://bugzilla.kernel.org/show_bug.cgi?id=202927, here is the tail end

[   14.896337] NET: Registered protocol family 1
[   14.901314] initcall af_unix_init+0x0/0x4e returned 0 after 4866 usecs
[   14.908694] calling  ipv6_offload_init+0x0/0x7f @ 1
[   14.914238] initcall ipv6_offload_init+0x0/0x7f returned 0 after 1 usecs
[   14.921821] calling  vlan_offload_init+0x0/0x20 @ 1
[   14.927365] initcall vlan_offload_init+0x0/0x20 returned 0 after 0 usecs
[   14.934948] calling  pci_apply_final_quirks+0x0/0x126 @ 1
[   14.941106] pci :00:1a.0: calling  quirk_usb_early_handoff+0x0/0x6a0 @ 1

thanks, Jesse Hathaway

Re: Regression causes a hang on boot with a Comtrol PCI card

[Jesse Hathaway]

> > 1302fcf0d03e (refs/bisect/bad) PCI: Configure *all* devices, not just
> > hot-added ones
> > 1c3c5eab1715 sched/core: Enable might_sleep() and smp_processor_id()
> > checks early
>
> How did you narrow it down to *two* commits, and do you have to revert
> both of them to avoid the hang?  Usually a bisection identifies a
> single commit, and the two you mention aren't related.

Sorry I should have been more verbose in what the bisection process was, I
found the problem after attempting to upgrade from linux v3.16 to v4.9. When
v4.9 hung I tried the latest kernel, v5.0, which also hanged. I began a git
bisect, but found there was more than one bad commit. Here is my current
understanding:

- [x] v3.18 vanilla, 1302fcf0d03e committed, hangs
- [x] v3.18 with revert of 1302fcf0d03e, works
.
.
.
- [x] v4.12 vanilla, hangs
- [x] v4.12 with revert of 1302fcf0d03e, works

- [x] v4.13 vanilla, 1c3c5eab1715 committed, hangs
- [x] v4.13 with revert of 1302fcf0d03e, hangs
- [x] v4.13 with revert of 1c3c5eab1715, hangs
- [x] v4.13 with revert of 1302fcf0d03e & 1c3c5eab1715, works

- [x] v5.0 vanilla, hangs
- [x] v5.0 with revert of 1302fcf0d03e & 1c3c5eab1715, works

> Can you collect a complete dmesg log (with a working kernel) and
> output of "sudo lspci -vvxxx"?  You can open a bug report at
> https://bugzilla.kernel.org, attach the logs there, and respond here
> with the URL.

Bug submitted along with the requested logs,
https://bugzilla.kernel.org/show_bug.cgi?id=202927

> Where does the hang happen?  Is it when we configure the Comtrol card?

Hang occurs after PCI is initialized, snippet below, I have included the full
output in the bug report:

[   10.561971] pci :81:00.0:   bridge window [mem 0xc800-0xc80f]
[   10.569661] pci :80:01.0: PCI bridge to [bus 81-82]
[   10.575594] pci :80:01.0:   bridge window [mem 0xc800-0xc80f]
[   10.583278] pci :80:03.0: PCI bridge to [bus 83]
[   10.589008] NET: Registered protocol family 2
[   10.594254] tcp_listen_portaddr_hash hash table entries: 65536
(order: 8, 1048576 bytes)
[   10.603671] TCP established hash table entries: 524288 (order: 10,
4194304 bytes)
[   10.612729] TCP bind hash table entries: 65536 (order: 8, 1048576 bytes)
[   10.620446] TCP: Hash tables configured (established 524288 bind 65536)
[   10.628124] UDP hash table entries: 65536 (order: 9, 2097152 bytes)
[   10.635541] UDP-Lite hash table entries: 65536 (order: 9, 2097152 bytes)
[   10.643669] NET: Registered protocol family 1

Please let me know if there is anything else I can provide, I am also happy to
test any patches, Jesse Hathaway

Regression causes a hang on boot with a Comtrol PCI card

[Jesse Hathaway]

Two regressions cause Linux to hang on boot when a Comtrol PCI card is present.

If I revert the following two commits, I can boot again and the card operates
without issue:

1302fcf0d03e (refs/bisect/bad) PCI: Configure *all* devices, not just
hot-added ones
1c3c5eab1715 sched/core: Enable might_sleep() and smp_processor_id()
checks early

; lspci -vs 82:00.0
82:00.0 Multiport serial controller: Comtrol Corporation Device 0061
Subsystem: Comtrol Corporation Device 0061
Flags: 66MHz, medium devsel, IRQ 35, NUMA node 1
Memory at c8004000 (32-bit, non-prefetchable) [size=4K]
Memory at c800 (32-bit, non-prefetchable) [size=16K]
Capabilities: [40] Hot-plug capable
Capabilities: [48] Power Management version 2
Kernel driver in use: rp2
Kernel modules: rp2

Is it possible that the problem is that the card claims to support Hot-plug,
but does not?

I would love to help fix this issue, please let me know what other information
would be helpful to provide.

; awk -f scripts/ver_linux
If some fields are empty or look unusual you may have an old version.
Compare to the current minimal requirements in Documentation/Changes.

Linux tty01 5.0.1-amd64 #1 SMP Wed Mar 13 15:43:44 UTC 2019 x86_64 GNU/Linux

GNU C   6.3.0
GNU Make4.1
Binutils2.28
Util-linux  2.29.2
Mount   2.29.2
Linux C Library 2.24
Dynamic linker (ldd)2.24
Procps  3.3.12
Sh-utils8.26
Udev232
Modules Loaded  8021q acpi_power_meter aesni_intel aes_x86_64
ahci autofs4 bonding button coretemp crc16 crc32c_generic crc32c_intel
crc32_pclmul crct10dif_pclmul cryptd crypto_simd dca dcdbas dm_mod drm
drm_kms_helper ehci_hcd ehci_pci evdev ext4 fscrypto garp
ghash_clmulni_intel glue_helper i2c_algo_bit igb intel_cstate
intel_powerclamp intel_rapl intel_rapl_perf intel_uncore ioatdma
ipmi_devintf ipmi_msghandler ipmi_si iptable_filter ip_tables
irqbypass iTCO_vendor_support iTCO_wdt ixgbe jbd2 kvm kvm_intel
libahci libata libcrc32c libphy llc lpc_ich mbcache mdio megaraid_sas
mei mei_me mgag200 mrp mxm_wmi nf_conntrack nf_defrag_ipv4
nf_defrag_ipv6 pcc_cpufreq pcspkr rp2 sb_edac scsi_mod sd_mod sg snd
snd_pcm snd_timer soundcore stp ttm usbcore wmi x86_pkg_temp_thermal
xfrm_algo x_tables xt_conntrack xt_tcpudp

Retouching added

[Jesse Kurtz]

Just want to check with you to see if you have any photos for editing?

We are providing below services to you.
Clipping path for the images.
Cut out for the photos
Masking for the images
All kinds of retouching for the beauty and model.

We provide testing for your photos if you need.

Thanks,
Jesse

Retouching added

[Jesse Kurtz]

Just want to check with you to see if you have any photos for editing?

We are providing below services to you.
Clipping path for the images.
Cut out for the photos
Masking for the images
All kinds of retouching for the beauty and model.

We provide testing for your photos if you need.

Thanks,
Jesse

Retouching added

[Jesse Kurtz]

Just want to check with you to see if you have any photos for editing?

We are providing below services to you.
Clipping path for the images.
Cut out for the photos
Masking for the images
All kinds of retouching for the beauty and model.

We provide testing for your photos if you need.

Thanks,
Jesse

Retouching added

[Jesse Kurtz]

Just want to check with you to see if you have any photos for editing?

We are providing below services to you.
Clipping path for the images.
Cut out for the photos
Masking for the images
All kinds of retouching for the beauty and model.

We provide testing for your photos if you need.

Thanks,
Jesse

Replace your photos

[Jesse Kurtz]

Just want to check with you to see if you have any photos for editing?

We are providing below services to you.
Clipping path for the images.
Cut out for the photos
Masking for the images
All kinds of retouching for the beauty and model.

We provide testing for your photos if you need.

Thanks,
Jesse

Replace your photos

[Jesse Kurtz]

Just want to check with you to see if you have any photos for editing?

We are providing below services to you.
Clipping path for the images.
Cut out for the photos
Masking for the images
All kinds of retouching for the beauty and model.

We provide testing for your photos if you need.

Thanks,
Jesse

Photo replacing

[Jesse Kurtz]

Just want to check with you to see if you have any photos for editing?

We are providing below services to you.
Clipping path for the images.
Cut out for the photos
Masking for the images
All kinds of retouching for the beauty and model.

We provide testing for your photos if you need.

Thanks,
Jesse

Photo replacing

[Jesse Kurtz]

Just want to check with you to see if you have any photos for editing?

We are providing below services to you.
Clipping path for the images.
Cut out for the photos
Masking for the images
All kinds of retouching for the beauty and model.

We provide testing for your photos if you need.

Thanks,
Jesse

Re: [RFC PATCH net-next 10/12] vhost_net: build xdp buff

[Jesse Brandeburg]

On Mon, 21 May 2018 17:04:31 +0800 Jason wrote:
> This patch implement build XDP buffers in vhost_net. The idea is do
> userspace copy in vhost_net and build XDP buff based on the
> page. Vhost_net can then submit one or an array of XDP buffs to
> underlayer socket (e.g TUN). TUN can choose to do XDP or call
> build_skb() to build skb. To support build skb, vnet header were also
> stored into the header of the XDP buff.
> 
> This userspace copy and XDP buffs building is key to achieve XDP
> batching in TUN, since TUN does not need to care about userspace copy
> and then can disable premmption for several XDP buffs to achieve
> batching from XDP.
> 
> TODO: reserve headroom based on the TUN XDP.
> 
> Signed-off-by: Jason Wang 
> ---
>  drivers/vhost/net.c | 74 
> +
>  1 file changed, 74 insertions(+)
> 
> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> index f0639d7..1209e84 100644
> --- a/drivers/vhost/net.c
> +++ b/drivers/vhost/net.c
> @@ -492,6 +492,80 @@ static bool vhost_has_more_pkts(struct vhost_net *net,
>  likely(!vhost_exceeds_maxpend(net));
>  }
>  
> +#define VHOST_NET_HEADROOM 256
> +#define VHOST_NET_RX_PAD (NET_IP_ALIGN + NET_SKB_PAD)
> +
> +static int vhost_net_build_xdp(struct vhost_net_virtqueue *nvq,
> +struct iov_iter *from,
> +struct xdp_buff *xdp)
> +{
> + struct vhost_virtqueue *vq = >vq;
> + struct page_frag *alloc_frag = >task_frag;
> + struct virtio_net_hdr *gso;
> + size_t len = iov_iter_count(from);
> + int buflen = SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
> + int pad = SKB_DATA_ALIGN(VHOST_NET_RX_PAD + VHOST_NET_HEADROOM
> +  + nvq->sock_hlen);
> + int sock_hlen = nvq->sock_hlen;
> + void *buf;
> + int copied;
> +
> + if (len < nvq->sock_hlen)
> + return -EFAULT;
> +
> + if (SKB_DATA_ALIGN(len + pad) +
> + SKB_DATA_ALIGN(sizeof(struct skb_shared_info)) > PAGE_SIZE)
> + return -ENOSPC;
> +
> + buflen += SKB_DATA_ALIGN(len + pad);

maybe store the result of SKB_DATA_ALIGN in a local instead of doing
the work twice?

> + alloc_frag->offset = ALIGN((u64)alloc_frag->offset, SMP_CACHE_BYTES);
> + if (unlikely(!skb_page_frag_refill(buflen, alloc_frag, GFP_KERNEL)))
> + return -ENOMEM;
> +
> + buf = (char *)page_address(alloc_frag->page) + alloc_frag->offset;
> +
> + /* We store two kinds of metadata in the header which will be
> +  * used for XDP_PASS to do build_skb():
> +  * offset 0: buflen
> +  * offset sizeof(int): vnet header
> +  */
> + copied = copy_page_from_iter(alloc_frag->page,
> +  alloc_frag->offset + sizeof(int), 
> sock_hlen, from);
> + if (copied != sock_hlen)
> + return -EFAULT;
> +
> + gso = (struct virtio_net_hdr *)(buf + sizeof(int));
> +
> + if ((gso->flags & VIRTIO_NET_HDR_F_NEEDS_CSUM) &&
> + vhost16_to_cpu(vq, gso->csum_start) +
> + vhost16_to_cpu(vq, gso->csum_offset) + 2 >
> + vhost16_to_cpu(vq, gso->hdr_len)) {
> + gso->hdr_len = cpu_to_vhost16(vq,
> +vhost16_to_cpu(vq, gso->csum_start) +
> +vhost16_to_cpu(vq, gso->csum_offset) + 2);
> +
> + if (vhost16_to_cpu(vq, gso->hdr_len) > len)
> + return -EINVAL;
> + }
> +
> + len -= sock_hlen;
> + copied = copy_page_from_iter(alloc_frag->page,
> +  alloc_frag->offset + pad,
> +  len, from);
> + if (copied != len)
> + return -EFAULT;
> +
> + xdp->data_hard_start = buf;
> + xdp->data = buf + pad;
> + xdp->data_end = xdp->data + len;
> + *(int *)(xdp->data_hard_start)= buflen;

space before =

> +
> + get_page(alloc_frag->page);
> + alloc_frag->offset += buflen;
> +
> + return 0;
> +}
> +
>  static void handle_tx_copy(struct vhost_net *net)
>  {
>   struct vhost_net_virtqueue *nvq = >vqs[VHOST_NET_VQ_TX];

Re: [RFC PATCH net-next 10/12] vhost_net: build xdp buff

[Jesse Brandeburg]

On Mon, 21 May 2018 17:04:31 +0800 Jason wrote:
> This patch implement build XDP buffers in vhost_net. The idea is do
> userspace copy in vhost_net and build XDP buff based on the
> page. Vhost_net can then submit one or an array of XDP buffs to
> underlayer socket (e.g TUN). TUN can choose to do XDP or call
> build_skb() to build skb. To support build skb, vnet header were also
> stored into the header of the XDP buff.
> 
> This userspace copy and XDP buffs building is key to achieve XDP
> batching in TUN, since TUN does not need to care about userspace copy
> and then can disable premmption for several XDP buffs to achieve
> batching from XDP.
> 
> TODO: reserve headroom based on the TUN XDP.
> 
> Signed-off-by: Jason Wang 
> ---
>  drivers/vhost/net.c | 74 
> +
>  1 file changed, 74 insertions(+)
> 
> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> index f0639d7..1209e84 100644
> --- a/drivers/vhost/net.c
> +++ b/drivers/vhost/net.c
> @@ -492,6 +492,80 @@ static bool vhost_has_more_pkts(struct vhost_net *net,
>  likely(!vhost_exceeds_maxpend(net));
>  }
>  
> +#define VHOST_NET_HEADROOM 256
> +#define VHOST_NET_RX_PAD (NET_IP_ALIGN + NET_SKB_PAD)
> +
> +static int vhost_net_build_xdp(struct vhost_net_virtqueue *nvq,
> +struct iov_iter *from,
> +struct xdp_buff *xdp)
> +{
> + struct vhost_virtqueue *vq = >vq;
> + struct page_frag *alloc_frag = >task_frag;
> + struct virtio_net_hdr *gso;
> + size_t len = iov_iter_count(from);
> + int buflen = SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
> + int pad = SKB_DATA_ALIGN(VHOST_NET_RX_PAD + VHOST_NET_HEADROOM
> +  + nvq->sock_hlen);
> + int sock_hlen = nvq->sock_hlen;
> + void *buf;
> + int copied;
> +
> + if (len < nvq->sock_hlen)
> + return -EFAULT;
> +
> + if (SKB_DATA_ALIGN(len + pad) +
> + SKB_DATA_ALIGN(sizeof(struct skb_shared_info)) > PAGE_SIZE)
> + return -ENOSPC;
> +
> + buflen += SKB_DATA_ALIGN(len + pad);

maybe store the result of SKB_DATA_ALIGN in a local instead of doing
the work twice?

> + alloc_frag->offset = ALIGN((u64)alloc_frag->offset, SMP_CACHE_BYTES);
> + if (unlikely(!skb_page_frag_refill(buflen, alloc_frag, GFP_KERNEL)))
> + return -ENOMEM;
> +
> + buf = (char *)page_address(alloc_frag->page) + alloc_frag->offset;
> +
> + /* We store two kinds of metadata in the header which will be
> +  * used for XDP_PASS to do build_skb():
> +  * offset 0: buflen
> +  * offset sizeof(int): vnet header
> +  */
> + copied = copy_page_from_iter(alloc_frag->page,
> +  alloc_frag->offset + sizeof(int), 
> sock_hlen, from);
> + if (copied != sock_hlen)
> + return -EFAULT;
> +
> + gso = (struct virtio_net_hdr *)(buf + sizeof(int));
> +
> + if ((gso->flags & VIRTIO_NET_HDR_F_NEEDS_CSUM) &&
> + vhost16_to_cpu(vq, gso->csum_start) +
> + vhost16_to_cpu(vq, gso->csum_offset) + 2 >
> + vhost16_to_cpu(vq, gso->hdr_len)) {
> + gso->hdr_len = cpu_to_vhost16(vq,
> +vhost16_to_cpu(vq, gso->csum_start) +
> +vhost16_to_cpu(vq, gso->csum_offset) + 2);
> +
> + if (vhost16_to_cpu(vq, gso->hdr_len) > len)
> + return -EINVAL;
> + }
> +
> + len -= sock_hlen;
> + copied = copy_page_from_iter(alloc_frag->page,
> +  alloc_frag->offset + pad,
> +  len, from);
> + if (copied != len)
> + return -EFAULT;
> +
> + xdp->data_hard_start = buf;
> + xdp->data = buf + pad;
> + xdp->data_end = xdp->data + len;
> + *(int *)(xdp->data_hard_start)= buflen;

space before =

> +
> + get_page(alloc_frag->page);
> + alloc_frag->offset += buflen;
> +
> + return 0;
> +}
> +
>  static void handle_tx_copy(struct vhost_net *net)
>  {
>   struct vhost_net_virtqueue *nvq = >vqs[VHOST_NET_VQ_TX];

Re: [RFC PATCH net-next 04/12] vhost_net: split out datacopy logic

[Jesse Brandeburg]

On Mon, 21 May 2018 17:04:25 +0800 Jason wrote:
> Instead of mixing zerocopy and datacopy logics, this patch tries to
> split datacopy logic out. This results for a more compact code and
> specific optimization could be done on top more easily.
> 
> Signed-off-by: Jason Wang 
> ---
>  drivers/vhost/net.c | 111 
> +++-
>  1 file changed, 102 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> index 4ebac76..4682fcc 100644
> --- a/drivers/vhost/net.c
> +++ b/drivers/vhost/net.c
> @@ -492,9 +492,95 @@ static bool vhost_has_more_pkts(struct vhost_net *net,
>  likely(!vhost_exceeds_maxpend(net));
>  }
>  
> +static void handle_tx_copy(struct vhost_net *net)
> +{
> + struct vhost_net_virtqueue *nvq = >vqs[VHOST_NET_VQ_TX];
> + struct vhost_virtqueue *vq = >vq;
> + unsigned out, in;

move "out" and "in" down to inside the for loop as well.

> + int head;

move this "head" declaration inside for-loop below.

> + struct msghdr msg = {
> + .msg_name = NULL,
> + .msg_namelen = 0,
> + .msg_control = NULL,
> + .msg_controllen = 0,
> + .msg_flags = MSG_DONTWAIT,
> + };
> + size_t len, total_len = 0;
> + int err;
> + size_t hdr_size;
> + struct socket *sock;
> + struct vhost_net_ubuf_ref *uninitialized_var(ubufs);
> + int sent_pkts = 0;

why do we need so many locals?

> +
> + mutex_lock(>mutex);
> + sock = vq->private_data;
> + if (!sock)
> + goto out;
> +
> + if (!vq_iotlb_prefetch(vq))
> + goto out;
> +
> + vhost_disable_notify(>dev, vq);
> + vhost_net_disable_vq(net, vq);
> +
> + hdr_size = nvq->vhost_hlen;
> +
> + for (;;) {
> + head = vhost_net_tx_get_vq_desc(net, vq, vq->iov,
> + ARRAY_SIZE(vq->iov),
> + , );
> + /* On error, stop handling until the next kick. */
> + if (unlikely(head < 0))
> + break;
> + /* Nothing new?  Wait for eventfd to tell us they refilled. */
> + if (head == vq->num) {
> + if (unlikely(vhost_enable_notify(>dev, vq))) {
> + vhost_disable_notify(>dev, vq);
> + continue;
> + }
> + break;
> + }
> + if (in) {
> + vq_err(vq, "Unexpected descriptor format for TX: "
> +"out %d, int %d\n", out, in);

don't break strings, keep all the bits between " " together, even if it
is longer than 80 chars.

> + break;
> + }
> +
> + len = init_iov_iter(vq, _iter, hdr_size, out);
> + if (len < 0)
> + break;

same comment as previous patch, len is a size_t which is unsigned.

> +
> + total_len += len;
> + if (total_len < VHOST_NET_WEIGHT &&
> + vhost_has_more_pkts(net, vq)) {
> + msg.msg_flags |= MSG_MORE;
> + } else {
> + msg.msg_flags &= ~MSG_MORE;
> + }

don't need { } here.

> +
> + /* TODO: Check specific error and bomb out unless ENOBUFS? */
> + err = sock->ops->sendmsg(sock, , len);
> + if (unlikely(err < 0)) {
> + vhost_discard_vq_desc(vq, 1);
> + vhost_net_enable_vq(net, vq);
> + break;
> + }
> + if (err != len)
> + pr_debug("Truncated TX packet: "
> +  " len %d != %zd\n", err, len);

single line " " string please

Re: [RFC PATCH net-next 04/12] vhost_net: split out datacopy logic

[Jesse Brandeburg]

On Mon, 21 May 2018 17:04:25 +0800 Jason wrote:
> Instead of mixing zerocopy and datacopy logics, this patch tries to
> split datacopy logic out. This results for a more compact code and
> specific optimization could be done on top more easily.
> 
> Signed-off-by: Jason Wang 
> ---
>  drivers/vhost/net.c | 111 
> +++-
>  1 file changed, 102 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> index 4ebac76..4682fcc 100644
> --- a/drivers/vhost/net.c
> +++ b/drivers/vhost/net.c
> @@ -492,9 +492,95 @@ static bool vhost_has_more_pkts(struct vhost_net *net,
>  likely(!vhost_exceeds_maxpend(net));
>  }
>  
> +static void handle_tx_copy(struct vhost_net *net)
> +{
> + struct vhost_net_virtqueue *nvq = >vqs[VHOST_NET_VQ_TX];
> + struct vhost_virtqueue *vq = >vq;
> + unsigned out, in;

move "out" and "in" down to inside the for loop as well.

> + int head;

move this "head" declaration inside for-loop below.

> + struct msghdr msg = {
> + .msg_name = NULL,
> + .msg_namelen = 0,
> + .msg_control = NULL,
> + .msg_controllen = 0,
> + .msg_flags = MSG_DONTWAIT,
> + };
> + size_t len, total_len = 0;
> + int err;
> + size_t hdr_size;
> + struct socket *sock;
> + struct vhost_net_ubuf_ref *uninitialized_var(ubufs);
> + int sent_pkts = 0;

why do we need so many locals?

> +
> + mutex_lock(>mutex);
> + sock = vq->private_data;
> + if (!sock)
> + goto out;
> +
> + if (!vq_iotlb_prefetch(vq))
> + goto out;
> +
> + vhost_disable_notify(>dev, vq);
> + vhost_net_disable_vq(net, vq);
> +
> + hdr_size = nvq->vhost_hlen;
> +
> + for (;;) {
> + head = vhost_net_tx_get_vq_desc(net, vq, vq->iov,
> + ARRAY_SIZE(vq->iov),
> + , );
> + /* On error, stop handling until the next kick. */
> + if (unlikely(head < 0))
> + break;
> + /* Nothing new?  Wait for eventfd to tell us they refilled. */
> + if (head == vq->num) {
> + if (unlikely(vhost_enable_notify(>dev, vq))) {
> + vhost_disable_notify(>dev, vq);
> + continue;
> + }
> + break;
> + }
> + if (in) {
> + vq_err(vq, "Unexpected descriptor format for TX: "
> +"out %d, int %d\n", out, in);

don't break strings, keep all the bits between " " together, even if it
is longer than 80 chars.

> + break;
> + }
> +
> + len = init_iov_iter(vq, _iter, hdr_size, out);
> + if (len < 0)
> + break;

same comment as previous patch, len is a size_t which is unsigned.

> +
> + total_len += len;
> + if (total_len < VHOST_NET_WEIGHT &&
> + vhost_has_more_pkts(net, vq)) {
> + msg.msg_flags |= MSG_MORE;
> + } else {
> + msg.msg_flags &= ~MSG_MORE;
> + }

don't need { } here.

> +
> + /* TODO: Check specific error and bomb out unless ENOBUFS? */
> + err = sock->ops->sendmsg(sock, , len);
> + if (unlikely(err < 0)) {
> + vhost_discard_vq_desc(vq, 1);
> + vhost_net_enable_vq(net, vq);
> + break;
> + }
> + if (err != len)
> + pr_debug("Truncated TX packet: "
> +  " len %d != %zd\n", err, len);

single line " " string please

Re: [RFC PATCH net-next 03/12] vhost_net: introduce vhost_has_more_pkts()

[Jesse Brandeburg]

On Mon, 21 May 2018 17:04:24 +0800 Jason wrote:
> Signed-off-by: Jason Wang 
> ---
>  drivers/vhost/net.c | 12 +---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> index de544ee..4ebac76 100644
> --- a/drivers/vhost/net.c
> +++ b/drivers/vhost/net.c
> @@ -485,6 +485,13 @@ static bool vhost_exceeds_weight(int pkts, int total_len)
>  unlikely(pkts >= VHOST_NET_PKT_WEIGHT);
>  }
>  
> +static bool vhost_has_more_pkts(struct vhost_net *net,
> + struct vhost_virtqueue *vq)
> +{
> + return !vhost_vq_avail_empty(>dev, vq) &&
> +likely(!vhost_exceeds_maxpend(net));

This really seems like mis-use of likely/unlikely, in the middle of a
sequence of operations that will always be run when this function is
called.  I think you should remove the likely from this helper,
especially, and control the branch from the branch point.


> +}
> +
>  /* Expects to be always run from workqueue - which acts as
>   * read-size critical section for our kind of RCU. */
>  static void handle_tx(struct vhost_net *net)
> @@ -578,8 +585,7 @@ static void handle_tx(struct vhost_net *net)
>   }
>   total_len += len;
>   if (total_len < VHOST_NET_WEIGHT &&
> - !vhost_vq_avail_empty(>dev, vq) &&
> - likely(!vhost_exceeds_maxpend(net))) {
> + vhost_has_more_pkts(net, vq)) {

Yes, I know it came from here, but likely/unlikely are for branch
control, so they should encapsulate everything inside the if, unless
I'm mistaken.

>   msg.msg_flags |= MSG_MORE;
>   } else {
>   msg.msg_flags &= ~MSG_MORE;
> @@ -605,7 +611,7 @@ static void handle_tx(struct vhost_net *net)
>   else
>   vhost_zerocopy_signal_used(net, vq);
>   vhost_net_tx_packet(net);
> - if (unlikely(vhost_exceeds_weight(++sent_pkts, total_len))) {
> + if (vhost_exceeds_weight(++sent_pkts, total_len)) {

You should have kept the unlikely here, and not had it inside the
helper (as per the previous patch.  Also, why wasn't this change part
of the previous patch?

>   vhost_poll_queue(>poll);
>   break;
>   }

Re: [RFC PATCH net-next 03/12] vhost_net: introduce vhost_has_more_pkts()

[Jesse Brandeburg]

On Mon, 21 May 2018 17:04:24 +0800 Jason wrote:
> Signed-off-by: Jason Wang 
> ---
>  drivers/vhost/net.c | 12 +---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> index de544ee..4ebac76 100644
> --- a/drivers/vhost/net.c
> +++ b/drivers/vhost/net.c
> @@ -485,6 +485,13 @@ static bool vhost_exceeds_weight(int pkts, int total_len)
>  unlikely(pkts >= VHOST_NET_PKT_WEIGHT);
>  }
>  
> +static bool vhost_has_more_pkts(struct vhost_net *net,
> + struct vhost_virtqueue *vq)
> +{
> + return !vhost_vq_avail_empty(>dev, vq) &&
> +likely(!vhost_exceeds_maxpend(net));

This really seems like mis-use of likely/unlikely, in the middle of a
sequence of operations that will always be run when this function is
called.  I think you should remove the likely from this helper,
especially, and control the branch from the branch point.


> +}
> +
>  /* Expects to be always run from workqueue - which acts as
>   * read-size critical section for our kind of RCU. */
>  static void handle_tx(struct vhost_net *net)
> @@ -578,8 +585,7 @@ static void handle_tx(struct vhost_net *net)
>   }
>   total_len += len;
>   if (total_len < VHOST_NET_WEIGHT &&
> - !vhost_vq_avail_empty(>dev, vq) &&
> - likely(!vhost_exceeds_maxpend(net))) {
> + vhost_has_more_pkts(net, vq)) {

Yes, I know it came from here, but likely/unlikely are for branch
control, so they should encapsulate everything inside the if, unless
I'm mistaken.

>   msg.msg_flags |= MSG_MORE;
>   } else {
>   msg.msg_flags &= ~MSG_MORE;
> @@ -605,7 +611,7 @@ static void handle_tx(struct vhost_net *net)
>   else
>   vhost_zerocopy_signal_used(net, vq);
>   vhost_net_tx_packet(net);
> - if (unlikely(vhost_exceeds_weight(++sent_pkts, total_len))) {
> + if (vhost_exceeds_weight(++sent_pkts, total_len)) {

You should have kept the unlikely here, and not had it inside the
helper (as per the previous patch.  Also, why wasn't this change part
of the previous patch?

>   vhost_poll_queue(>poll);
>   break;
>   }

Re: [RFC PATCH net-next 02/12] vhost_net: introduce vhost_exceeds_weight()

[Jesse Brandeburg]

On Mon, 21 May 2018 17:04:23 +0800 Jason wrote:
> Signed-off-by: Jason Wang 
> ---
>  drivers/vhost/net.c | 13 -
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> index 15d191a..de544ee 100644
> --- a/drivers/vhost/net.c
> +++ b/drivers/vhost/net.c
> @@ -479,6 +479,12 @@ static size_t init_iov_iter(struct vhost_virtqueue *vq, 
> struct iov_iter *iter,
>   return len;
>  }
>  
> +static bool vhost_exceeds_weight(int pkts, int total_len)
> +{
> + return unlikely(total_len >= VHOST_NET_WEIGHT) ||
> +unlikely(pkts >= VHOST_NET_PKT_WEIGHT);

I was going to say just one unlikely, but then the caller of this
function also says unlikely(vhost_exceeds...), so I think you should
just drop the unlikely statements here (both of them)

> +}
> +
>  /* Expects to be always run from workqueue - which acts as
>   * read-size critical section for our kind of RCU. */
>  static void handle_tx(struct vhost_net *net)
> @@ -570,7 +576,6 @@ static void handle_tx(struct vhost_net *net)
>   msg.msg_control = NULL;
>   ubufs = NULL;
>   }
> -

unrelated whitespace changes?

>   total_len += len;
>   if (total_len < VHOST_NET_WEIGHT &&
>   !vhost_vq_avail_empty(>dev, vq) &&
> @@ -600,8 +605,7 @@ static void handle_tx(struct vhost_net *net)
>   else
>   vhost_zerocopy_signal_used(net, vq);
>   vhost_net_tx_packet(net);
> - if (unlikely(total_len >= VHOST_NET_WEIGHT) ||
> - unlikely(++sent_pkts >= VHOST_NET_PKT_WEIGHT)) {
> + if (unlikely(vhost_exceeds_weight(++sent_pkts, total_len))) {
>   vhost_poll_queue(>poll);
>   break;
>   }
> @@ -887,8 +891,7 @@ static void handle_rx(struct vhost_net *net)
>   if (unlikely(vq_log))
>   vhost_log_write(vq, vq_log, log, vhost_len);
>   total_len += vhost_len;
> - if (unlikely(total_len >= VHOST_NET_WEIGHT) ||
> - unlikely(++recv_pkts >= VHOST_NET_PKT_WEIGHT)) {
> + if (unlikely(vhost_exceeds_weight(++recv_pkts, total_len))) {
>   vhost_poll_queue(>poll);
>   goto out;
>   }

Re: [RFC PATCH net-next 02/12] vhost_net: introduce vhost_exceeds_weight()

[Jesse Brandeburg]

On Mon, 21 May 2018 17:04:23 +0800 Jason wrote:
> Signed-off-by: Jason Wang 
> ---
>  drivers/vhost/net.c | 13 -
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> index 15d191a..de544ee 100644
> --- a/drivers/vhost/net.c
> +++ b/drivers/vhost/net.c
> @@ -479,6 +479,12 @@ static size_t init_iov_iter(struct vhost_virtqueue *vq, 
> struct iov_iter *iter,
>   return len;
>  }
>  
> +static bool vhost_exceeds_weight(int pkts, int total_len)
> +{
> + return unlikely(total_len >= VHOST_NET_WEIGHT) ||
> +unlikely(pkts >= VHOST_NET_PKT_WEIGHT);

I was going to say just one unlikely, but then the caller of this
function also says unlikely(vhost_exceeds...), so I think you should
just drop the unlikely statements here (both of them)

> +}
> +
>  /* Expects to be always run from workqueue - which acts as
>   * read-size critical section for our kind of RCU. */
>  static void handle_tx(struct vhost_net *net)
> @@ -570,7 +576,6 @@ static void handle_tx(struct vhost_net *net)
>   msg.msg_control = NULL;
>   ubufs = NULL;
>   }
> -

unrelated whitespace changes?

>   total_len += len;
>   if (total_len < VHOST_NET_WEIGHT &&
>   !vhost_vq_avail_empty(>dev, vq) &&
> @@ -600,8 +605,7 @@ static void handle_tx(struct vhost_net *net)
>   else
>   vhost_zerocopy_signal_used(net, vq);
>   vhost_net_tx_packet(net);
> - if (unlikely(total_len >= VHOST_NET_WEIGHT) ||
> - unlikely(++sent_pkts >= VHOST_NET_PKT_WEIGHT)) {
> + if (unlikely(vhost_exceeds_weight(++sent_pkts, total_len))) {
>   vhost_poll_queue(>poll);
>   break;
>   }
> @@ -887,8 +891,7 @@ static void handle_rx(struct vhost_net *net)
>   if (unlikely(vq_log))
>   vhost_log_write(vq, vq_log, log, vhost_len);
>   total_len += vhost_len;
> - if (unlikely(total_len >= VHOST_NET_WEIGHT) ||
> - unlikely(++recv_pkts >= VHOST_NET_PKT_WEIGHT)) {
> + if (unlikely(vhost_exceeds_weight(++recv_pkts, total_len))) {
>   vhost_poll_queue(>poll);
>   goto out;
>   }

Re: [RFC PATCH net-next 01/12] vhost_net: introduce helper to initialize tx iov iter

[Jesse Brandeburg]

Hi Jason, a few nits. 

On Mon, 21 May 2018 17:04:22 +0800 Jason wrote:
> Signed-off-by: Jason Wang 
> ---
>  drivers/vhost/net.c | 34 +++---
>  1 file changed, 23 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> index c4b49fc..15d191a 100644
> --- a/drivers/vhost/net.c
> +++ b/drivers/vhost/net.c
> @@ -459,6 +459,26 @@ static bool vhost_exceeds_maxpend(struct vhost_net *net)
>  min_t(unsigned int, VHOST_MAX_PEND, vq->num >> 2);
>  }
>  
> +static size_t init_iov_iter(struct vhost_virtqueue *vq, struct iov_iter 
> *iter,
> + size_t hdr_size, int out)
> +{
> + /* Skip header. TODO: support TSO. */
> + size_t len = iov_length(vq->iov, out);
> +
> + iov_iter_init(iter, WRITE, vq->iov, out, len);
> + iov_iter_advance(iter, hdr_size);
> + /* Sanity check */
> + if (!iov_iter_count(iter)) {
> + vq_err(vq, "Unexpected header len for TX: "
> + "%zd expected %zd\n",
> + len, hdr_size);

ok, it was like this before, but please unwrap the string in " ", there
should be no line breaks in string declarations and they are allowed to
go over 80 characters.

> + return -EFAULT;
> + }
> + len = iov_iter_count(iter);
> +
> + return len;
> +}
> +
>  /* Expects to be always run from workqueue - which acts as
>   * read-size critical section for our kind of RCU. */
>  static void handle_tx(struct vhost_net *net)
> @@ -521,18 +541,10 @@ static void handle_tx(struct vhost_net *net)
>  "out %d, int %d\n", out, in);
>   break;
>   }
> - /* Skip header. TODO: support TSO. */
> - len = iov_length(vq->iov, out);
> - iov_iter_init(_iter, WRITE, vq->iov, out, len);
> - iov_iter_advance(_iter, hdr_size);
> - /* Sanity check */
> - if (!msg_data_left()) {
> - vq_err(vq, "Unexpected header len for TX: "
> -"%zd expected %zd\n",
> -len, hdr_size);
> +
> + len = init_iov_iter(vq, _iter, hdr_size, out);
> + if (len < 0)

len is declared as size_t, which is unsigned, and can never be
negative.  I'm pretty sure this is a bug.


>   break;
> - }
> - len = msg_data_left();
>  
>   zcopy_used = zcopy && len >= VHOST_GOODCOPY_LEN
>  && !vhost_exceeds_maxpend(net)

Re: [RFC PATCH net-next 01/12] vhost_net: introduce helper to initialize tx iov iter

[Jesse Brandeburg]

Hi Jason, a few nits. 

On Mon, 21 May 2018 17:04:22 +0800 Jason wrote:
> Signed-off-by: Jason Wang 
> ---
>  drivers/vhost/net.c | 34 +++---
>  1 file changed, 23 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> index c4b49fc..15d191a 100644
> --- a/drivers/vhost/net.c
> +++ b/drivers/vhost/net.c
> @@ -459,6 +459,26 @@ static bool vhost_exceeds_maxpend(struct vhost_net *net)
>  min_t(unsigned int, VHOST_MAX_PEND, vq->num >> 2);
>  }
>  
> +static size_t init_iov_iter(struct vhost_virtqueue *vq, struct iov_iter 
> *iter,
> + size_t hdr_size, int out)
> +{
> + /* Skip header. TODO: support TSO. */
> + size_t len = iov_length(vq->iov, out);
> +
> + iov_iter_init(iter, WRITE, vq->iov, out, len);
> + iov_iter_advance(iter, hdr_size);
> + /* Sanity check */
> + if (!iov_iter_count(iter)) {
> + vq_err(vq, "Unexpected header len for TX: "
> + "%zd expected %zd\n",
> + len, hdr_size);

ok, it was like this before, but please unwrap the string in " ", there
should be no line breaks in string declarations and they are allowed to
go over 80 characters.

> + return -EFAULT;
> + }
> + len = iov_iter_count(iter);
> +
> + return len;
> +}
> +
>  /* Expects to be always run from workqueue - which acts as
>   * read-size critical section for our kind of RCU. */
>  static void handle_tx(struct vhost_net *net)
> @@ -521,18 +541,10 @@ static void handle_tx(struct vhost_net *net)
>  "out %d, int %d\n", out, in);
>   break;
>   }
> - /* Skip header. TODO: support TSO. */
> - len = iov_length(vq->iov, out);
> - iov_iter_init(_iter, WRITE, vq->iov, out, len);
> - iov_iter_advance(_iter, hdr_size);
> - /* Sanity check */
> - if (!msg_data_left()) {
> - vq_err(vq, "Unexpected header len for TX: "
> -"%zd expected %zd\n",
> -len, hdr_size);
> +
> + len = init_iov_iter(vq, _iter, hdr_size, out);
> + if (len < 0)

len is declared as size_t, which is unsigned, and can never be
negative.  I'm pretty sure this is a bug.


>   break;
> - }
> - len = msg_data_left();
>  
>   zcopy_used = zcopy && len >= VHOST_GOODCOPY_LEN
>  && !vhost_exceeds_maxpend(net)

Re: [uml-devel] [PATCH] uml: Fix build with recent glibc

[Jesse Brandeburg]

On Wed, 28 Feb 2018 19:08:44 -0800
Andi Kleen <a...@firstfloor.org> wrote:

> From: Andi Kleen <a...@linux.intel.com>
> 
> Newer glibc did some include namespace "cleanups" and removed
> struct ucontext and friends. This already broke a lot of software,
> and UML seems to be the latest victim.
> 
> Use the typedefs which are still available. They also work on
> older glibcs.
> 
> Signed-off-by: Andi Kleen <a...@linux.intel.com>

same patch that I sent on Feb 1st.  Hope you can get more traction than
I did.

https://www.mail-archive.com/user-mode-linux-devel@lists.sourceforge.net/msg10071.html

Reviewed-by: Jesse Brandeburg <jesse.brandeb...@intel.com>

Re: [uml-devel] [PATCH] uml: Fix build with recent glibc

[Jesse Brandeburg]

On Wed, 28 Feb 2018 19:08:44 -0800
Andi Kleen  wrote:

> From: Andi Kleen 
> 
> Newer glibc did some include namespace "cleanups" and removed
> struct ucontext and friends. This already broke a lot of software,
> and UML seems to be the latest victim.
> 
> Use the typedefs which are still available. They also work on
> older glibcs.
> 
> Signed-off-by: Andi Kleen 

same patch that I sent on Feb 1st.  Hope you can get more traction than
I did.

https://www.mail-archive.com/user-mode-linux-devel@lists.sourceforge.net/msg10071.html

Reviewed-by: Jesse Brandeburg 

[PATCH] um: fix user mode linux build

[Jesse Brandeburg]

Not sure when it got broken, but without this patch the command
make ARCH=um defconfig
make ARCH=um

fails due to struct ucontext being undefined.
arch/um/os-Linux/signal.c: In function ‘hard_handler’:
arch/um/os-Linux/signal.c:163:22: error: dereferencing pointer to incomplete 
type ‘struct ucontext’
  mcontext_t *mc = >uc_mcontext;


The fix seems fairly simple, that the code just needs to use
ucontext_t define instead.

I didn't verify that user-mode-linux is working after this change,
just that it builds successfully.

Cc: Jeff Dike <jd...@addtoit.com>
Cc: Richard Weinberger <rich...@nod.at>
Cc: user-mode-linux-de...@lists.sourceforge.net
Signed-off-by: Jesse Brandeburg <jesse.brandeb...@intel.com>
---
 arch/um/os-Linux/signal.c | 2 +-
 arch/x86/um/stub_segv.c   | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/um/os-Linux/signal.c b/arch/um/os-Linux/signal.c
index a86d7cc2c2d8..a5c0c909c48b 100644
--- a/arch/um/os-Linux/signal.c
+++ b/arch/um/os-Linux/signal.c
@@ -159,7 +159,7 @@ static void (*handlers[_NSIG])(int sig, struct siginfo *si, 
mcontext_t *mc) = {
 
 static void hard_handler(int sig, siginfo_t *si, void *p)
 {
-   struct ucontext *uc = p;
+   ucontext_t *uc = p;
mcontext_t *mc = >uc_mcontext;
unsigned long pending = 1UL << sig;
 
diff --git a/arch/x86/um/stub_segv.c b/arch/x86/um/stub_segv.c
index 1518d2805ae8..fd6825537b97 100644
--- a/arch/x86/um/stub_segv.c
+++ b/arch/x86/um/stub_segv.c
@@ -10,7 +10,7 @@
 void __attribute__ ((__section__ (".__syscall_stub")))
 stub_segv_handler(int sig, siginfo_t *info, void *p)
 {
-   struct ucontext *uc = p;
+   ucontext_t *uc = p;
 
GET_FAULTINFO_FROM_MC(*((struct faultinfo *) STUB_DATA),
  >uc_mcontext);
-- 
2.14.3

[PATCH] um: fix user mode linux build

[Jesse Brandeburg]

Not sure when it got broken, but without this patch the command
make ARCH=um defconfig
make ARCH=um

fails due to struct ucontext being undefined.
arch/um/os-Linux/signal.c: In function ‘hard_handler’:
arch/um/os-Linux/signal.c:163:22: error: dereferencing pointer to incomplete 
type ‘struct ucontext’
  mcontext_t *mc = >uc_mcontext;


The fix seems fairly simple, that the code just needs to use
ucontext_t define instead.

I didn't verify that user-mode-linux is working after this change,
just that it builds successfully.

Cc: Jeff Dike 
Cc: Richard Weinberger 
Cc: user-mode-linux-de...@lists.sourceforge.net
Signed-off-by: Jesse Brandeburg 
---
 arch/um/os-Linux/signal.c | 2 +-
 arch/x86/um/stub_segv.c   | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/um/os-Linux/signal.c b/arch/um/os-Linux/signal.c
index a86d7cc2c2d8..a5c0c909c48b 100644
--- a/arch/um/os-Linux/signal.c
+++ b/arch/um/os-Linux/signal.c
@@ -159,7 +159,7 @@ static void (*handlers[_NSIG])(int sig, struct siginfo *si, 
mcontext_t *mc) = {
 
 static void hard_handler(int sig, siginfo_t *si, void *p)
 {
-   struct ucontext *uc = p;
+   ucontext_t *uc = p;
mcontext_t *mc = >uc_mcontext;
unsigned long pending = 1UL << sig;
 
diff --git a/arch/x86/um/stub_segv.c b/arch/x86/um/stub_segv.c
index 1518d2805ae8..fd6825537b97 100644
--- a/arch/x86/um/stub_segv.c
+++ b/arch/x86/um/stub_segv.c
@@ -10,7 +10,7 @@
 void __attribute__ ((__section__ (".__syscall_stub")))
 stub_segv_handler(int sig, siginfo_t *info, void *p)
 {
-   struct ucontext *uc = p;
+   ucontext_t *uc = p;
 
GET_FAULTINFO_FROM_MC(*((struct faultinfo *) STUB_DATA),
  >uc_mcontext);
-- 
2.14.3

Re: [Intel-wired-lan] [PATCH] i40e: Delete an error message for a failed memory allocation in i40e_init_interrupt_scheme()

[Jesse Brandeburg]

On Mon, 1 Jan 2018 20:43:35 +0100
SF Markus Elfring <elfr...@users.sourceforge.net> wrote:

> From: Markus Elfring <elfr...@users.sourceforge.net>
> Date: Mon, 1 Jan 2018 20:38:14 +0100
> 
> Omit an extra message for a memory allocation failure in this function.
> 
> This issue was detected by using the Coccinelle software.
> 
> Signed-off-by: Markus Elfring <elfr...@users.sourceforge.net>

Thanks for the patch.

Acked-by: Jesse Brandeburg <jesse.brandeb...@intel.com>

Re: [Intel-wired-lan] [PATCH] i40e: Delete an error message for a failed memory allocation in i40e_init_interrupt_scheme()

[Jesse Brandeburg]

On Mon, 1 Jan 2018 20:43:35 +0100
SF Markus Elfring  wrote:

> From: Markus Elfring 
> Date: Mon, 1 Jan 2018 20:38:14 +0100
> 
> Omit an extra message for a memory allocation failure in this function.
> 
> This issue was detected by using the Coccinelle software.
> 
> Signed-off-by: Markus Elfring 

Thanks for the patch.

Acked-by: Jesse Brandeburg 

[PATCH] phy: qcom-ufs: add missing MODULE_DESCRIPTION/LICENSE

[Jesse Chan]

This change resolves a new compile-time warning
when built as a loadable module:

WARNING: modpost: missing MODULE_LICENSE() in 
drivers/phy/qualcomm/phy-qcom-ufs.o
see include/linux/module.h for more information

This adds the license as "GPL v2", which matches the header of the file.

MODULE_DESCRIPTION is also added.

Signed-off-by: Jesse Chan <j...@linux.com>
---
 drivers/phy/qualcomm/phy-qcom-ufs.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/phy/qualcomm/phy-qcom-ufs.c 
b/drivers/phy/qualcomm/phy-qcom-ufs.c
index c5ff4525edef..124dc70f6986 100644
--- a/drivers/phy/qualcomm/phy-qcom-ufs.c
+++ b/drivers/phy/qualcomm/phy-qcom-ufs.c
@@ -675,3 +675,6 @@ int ufs_qcom_phy_power_off(struct phy *generic_phy)
return 0;
 }
 EXPORT_SYMBOL_GPL(ufs_qcom_phy_power_off);
+
+MODULE_DESCRIPTION("Universal Flash Storage (UFS) QCOM PHY");
+MODULE_LICENSE("GPL v2");
-- 
2.14.1