* Re: [PATCH 19/20] drivers/net/ethernet/marvell/skge.c: fix error return code
From: Ezequiel Garcia @ 2012-10-10 17:40 UTC (permalink / raw)
To: Peter Senna Tschudin
Cc: Julia Lawall, Joe Perches, David Miller, shemminger, mlindner,
kernel-janitors, netdev, linux-kernel
In-Reply-To: <CA+MoWDp8Y0tJoBbz0nxH3Y7J0ur3TJm-dO9OM5BZVBKcbS9TpA@mail.gmail.com>
Hi Peter,
On Wed, Oct 10, 2012 at 2:08 PM, Peter Senna Tschudin
<peter.senna@gmail.com> wrote:
> Stephen and David,
>
> I've sent V2 of the patches and they were all accepted. Thank you.
>
> I've made a template for the commit message, and then copy and paste
> function names from the code. Something like:
>
> -- // --
> The function sky2_probe() return 0 for success and negative value
> for most of its internal tests failures. There are two exceptions
> that are error cases going to err_out*:. For this two cases, the
> function abort its success execution path, but returns non negative
> value, making it dificult for a caller function to notice the error.
>
> This patch fixes the error cases that do not return negative values.
>
> This was found by Coccinelle, but the code change was made by hand.
> This patch is not robot generated.
> ...
> --//--
>
> How useful it was to have the function names when you were analyzing
> the patches? It took me a lot of time to modify the template by copy
> and paste, check if it is correct, then commit. I have some other
> similar patches to submit and I wonder if having the function names in
> the commit message helped you.
>
Having real function names in your commit message won't make it more useful.
IMHO, the problem is you're still using a template commit message,
which produces
a robot-like commit message.
Developers don't like that, we prefer to see a text written by some
guy explaining
why is this patch needed, and what it's fixing/improving from an
overall point of view.
This is not easy and takes much training.
I believe patches should help maintainers, not only add work them,
so it's important to double-triple-check the patch and
double-triple-check the commit message.
I know this is tedious and it'll slow you a bit. But it's a good
thing: it means you are working :-)
Also, in this particular case, where the coccinelle does not fix
something obvious,
then I'd say you should be *extra* careful.
Hope this helps.
Ezequiel
^ permalink raw reply
* Re: setting flow spec rules under vswitch configuration
From: Ben Hutchings @ 2012-10-10 17:27 UTC (permalink / raw)
To: Or Gerlitz; +Cc: Rony Efraim, netdev, Amir Vadai
In-Reply-To: <507586B9.6080505@mellanox.com>
On Wed, 2012-10-10 at 16:31 +0200, Or Gerlitz wrote:
> On 10/10/2012 16:28, Ben Hutchings wrote:
> > On Wed, 2012-10-10 at 16:16 +0200, Or Gerlitz wrote:
> > [...]
> >> >Thanks for setting the sketch of a plan here... so if we go little bit
> >> >into details, we can safely move 20 bytes from the hadata[60] field into
> >> >the beginning of struct ethtool_flow_ext, which will still allow old
> >> >user space to work with newer kernels. As for newer uses space that
> >> >would like to set mac addresses within
> >> >ethtool_flow_ext, how are they supposed to identify if the kernel
> >> >supports this extension (of the extension...)?
> > Well they can't tell in advance, but we can define another flag in
> > flow_type like FLOW_EXT and existing drivers will reject flow specs with
> > that flag set.
> I understand what we can add a flag which will be rejected by driver's
> that don't support this ext,
> can the same/similar flag be used for newer user space code to mark they
> want this ext?
Exactly.
Ben.
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply
* Re: [PATCH 19/20] drivers/net/ethernet/marvell/skge.c: fix error return code
From: Peter Senna Tschudin @ 2012-10-10 17:08 UTC (permalink / raw)
To: Julia Lawall
Cc: Joe Perches, David Miller, shemminger, mlindner, kernel-janitors,
netdev, linux-kernel
In-Reply-To: <alpine.DEB.2.02.1210051003120.1958@localhost6.localdomain6>
Stephen and David,
I've sent V2 of the patches and they were all accepted. Thank you.
I've made a template for the commit message, and then copy and paste
function names from the code. Something like:
-- // --
The function sky2_probe() return 0 for success and negative value
for most of its internal tests failures. There are two exceptions
that are error cases going to err_out*:. For this two cases, the
function abort its success execution path, but returns non negative
value, making it dificult for a caller function to notice the error.
This patch fixes the error cases that do not return negative values.
This was found by Coccinelle, but the code change was made by hand.
This patch is not robot generated.
...
--//--
How useful it was to have the function names when you were analyzing
the patches? It took me a lot of time to modify the template by copy
and paste, check if it is correct, then commit. I have some other
similar patches to submit and I wonder if having the function names in
the commit message helped you.
Thank you,
Peter
On Fri, Oct 5, 2012 at 10:08 AM, Julia Lawall <julia.lawall@lip6.fr> wrote:
> On Fri, 5 Oct 2012, Joe Perches wrote:
>
>> On Fri, 2012-10-05 at 07:22 +0200, Julia Lawall wrote:
>>>
>>> A tool was used to find a potential problem, and then Peter
>>> studied the code to see what fix was appropriate.
>>
>>
>> Hi Julia.
>>
>> Was it true that a static analysis tool found the original
>> potential issue? If so, what tool was it?
>
>
> In the very beginning, I think that I found the problem in a patch when
> looking at patches that contain oopses.
>
> From that I wrote a Coccinelle rule. As Peter showed, the rule just
> produces a list of line numbers. The fix cannot easily be automated,
> because there are many cases where 0 is a valid error value. Some
> functions, for example, have their error value as a nonpositive integer.
>
>
>> But wasn't the scripted fix applied to the rest of the tree
>> robotically?
>
>
> No. Peter studied each case and considered what should be done, and then
> did that. I guess a potentially bad fix could have been applied
> automatically and then cleaned up manually, but considering the number of
> cases where the fix would be wrong, that seem like a bad idea. Also one
> might want to adapt a bit to local conventions about where the
> initialization should be added.
>
> julia
--
Peter
^ permalink raw reply
* Re: [1/2] PCI-Express Non-Transparent Bridge Support
From: Jon Mason @ 2012-10-10 17:06 UTC (permalink / raw)
To: Jakub Kicinski
Cc: linux-kernel, netdev, linux-pci, Dave Jiang, Nicholas Bellinger
In-Reply-To: <20121007141344.6624e873@wp.pl>
On Sun, Oct 07, 2012 at 02:13:44PM +0200, Jakub Kicinski wrote:
> Hi,
>
> it's good to see some NTB code getting into mainline! I have a few comments
> though.
>
> On Tue, 02 Oct 2012 21:26:16 -0000, Jon Mason <jon.mason@intel.com>
> wrote:
>
> [...]
> >+/**
> >+ * ntb_write_local_spad() - write to the secondary scratchpad register
> >+ * @ndev: pointer to ntb_device instance
> >+ * @idx: index to the scratchpad register, 0 based
> >+ * @val: the data value to put into the register
> >+ *
> >+ * This function allows writing of a 32bit value to the indexed scratchpad
> >+ * register. The register resides on the secondary (external) side.
> >+ *
> >+ * RETURNS: An appropriate -ERRNO error value on error, or zero for success.
> >+ */
> [...]
> >+/**
> >+ * ntb_write_remote_spad() - write to the secondary scratchpad register
> >+ * @ndev: pointer to ntb_device instance
> >+ * @idx: index to the scratchpad register, 0 based
> >+ * @val: the data value to put into the register
> >+ *
> >+ * This function allows writing of a 32bit value to the indexed scratchpad
> >+ * register. The register resides on the secondary (external) side.
> >+ *
> >+ * RETURNS: An appropriate -ERRNO error value on error, or zero for success.
> >+ */
>
> Those comments look suspiciously similar. I think one of the functions
> does write to primary scratchpad?
Yes, the comments can be improved.
>
> [...]
> >+/**
> >+ * ntb_read_local_spad() - read from the primary scratchpad register
> >+ * @ndev: pointer to ntb_device instance
> >+ * @idx: index to scratchpad register, 0 based
> >+ * @val: pointer to 32bit integer for storing the register value
> >+ *
> >+ * This function allows reading of the 32bit scratchpad register on
> >+ * the primary (internal) side.
> >+ *
> >+ * RETURNS: An appropriate -ERRNO error value on error, or zero for success.
> >+ */
> [...]
> >+/**
> >+ * ntb_read_remote_spad() - read from the primary scratchpad register
> >+ * @ndev: pointer to ntb_device instance
> >+ * @idx: index to scratchpad register, 0 based
> >+ * @val: pointer to 32bit integer for storing the register value
> >+ *
> >+ * This function allows reading of the 32bit scratchpad register on
> >+ * the primary (internal) side.
> >+ *
> >+ * RETURNS: An appropriate -ERRNO error value on error, or zero for success.
> >+ */
>
> Same here.
>
> [...]
> >+static int ntb_setup_msix(struct ntb_device *ndev)
> >+{
> >+ struct pci_dev *pdev = ndev->pdev;
> >+ struct msix_entry *msix;
> >+ int msix_entries;
> >+ int rc, i, pos;
> >+ u16 val;
> >+
> >+ pos = pci_find_capability(pdev, PCI_CAP_ID_MSIX);
> >+ if (!pos) {
> >+ rc = -EIO;
> >+ goto err;
> >+ }
> >+
> >+ rc = pci_read_config_word(pdev, pos + PCI_MSIX_FLAGS, &val);
> >+ if (rc)
> >+ goto err;
> >+
> >+ msix_entries = msix_table_size(val);
> >+ if (msix_entries > ndev->limits.msix_cnt) {
> >+ rc = -EINVAL;
> >+ goto err;
> >+ }
> >+
> >+ ndev->msix_entries = kmalloc(sizeof(struct msix_entry) * msix_entries,
> >+ GFP_KERNEL);
> >+ if (!ndev->msix_entries) {
> >+ rc = -ENOMEM;
> >+ goto err;
> >+ }
> >+
> >+ for (i = 0; i < msix_entries; i++)
> >+ ndev->msix_entries[i].entry = i;
> >+
> >+ rc = pci_enable_msix(pdev, ndev->msix_entries, msix_entries);
> >+ if (rc < 0)
> >+ goto err1;
> >+ if (rc > 0) {
>
> rc > 0 doesn't mean that vectors were allocated. Have a look at the
> example in Documentation/PCI/MSI-HOWTO.txt.
Thank you, I will correct this.
>
> >+ /* On SNB, the link interrupt is always tied to 4th vector. If
> >+ * we can't get all 4, then we can't use MSI-X.
> >+ */
> >+ if (ndev->hw_type != BWD_HW) {
> >+ rc = -EIO;
> >+ goto err1;
> >+ }
>
> This looks fragile, what if msix_table_size(val) was < 4?
If there are not at least 4 vectors, then we shouldn't use MSI-X.
>
> >+
> >+ dev_warn(&pdev->dev,
> >+ "Only %d MSI-X vectors. Limiting the number of queues to that number.\n",
> >+ rc);
> >+ msix_entries = rc;
> >+ }
> >+
> >+ for (i = 0; i < msix_entries; i++) {
> >+ msix = &ndev->msix_entries[i];
> >+ WARN_ON(!msix->vector);
> >+
> >+ /* Use the last MSI-X vector for Link status */
> >+ if (ndev->hw_type == BWD_HW) {
> >+ rc = request_irq(msix->vector, bwd_callback_msix_irq, 0,
> >+ "ntb-callback-msix", &ndev->db_cb[i]);
> >+ if (rc)
> >+ goto err2;
> >+ } else {
> >+ if (i == msix_entries - 1) {
> >+ rc = request_irq(msix->vector,
> >+ xeon_event_msix_irq, 0,
> >+ "ntb-event-msix", ndev);
> >+ if (rc)
> >+ goto err2;
> >+ } else {
> >+ rc = request_irq(msix->vector,
> >+ xeon_callback_msix_irq, 0,
> >+ "ntb-callback-msix",
> >+ &ndev->db_cb[i]);
> >+ if (rc)
> >+ goto err2;
> >+ }
> >+ }
> >+ }
> >+
> >+ ndev->num_msix = msix_entries;
> >+ if (ndev->hw_type == BWD_HW)
> >+ ndev->max_cbs = msix_entries;
> >+ else
> >+ ndev->max_cbs = msix_entries - 1;
> >+
> >+ return 0;
> >+
> >+err2:
> >+ while (--i >= 0) {
> >+ msix = &ndev->msix_entries[i];
> >+ if (ndev->hw_type != BWD_HW && i == ndev->num_msix - 1)
> >+ free_irq(msix->vector, ndev);
> >+ else
> >+ free_irq(msix->vector, &ndev->db_cb[i]);
> >+ }
> >+ pci_disable_msix(pdev);
> >+err1:
> >+ kfree(ndev->msix_entries);
> >+ dev_err(&pdev->dev, "Error allocating MSI-X interrupt\n");
> >+err:
> >+ ndev->num_msix = 0;
> >+ return rc;
> >+}
>
> Thanks for your work,
Thanks for the review.
>
> -- Kuba
^ permalink raw reply
* [PATCH V3] ath/ath9k/ar9003_eeprom.c: Remove semicolon after if
From: Peter Senna Tschudin @ 2012-10-10 16:38 UTC (permalink / raw)
To: mcgrof
Cc: jouni, vthiagar, senthilb, linville, linux-wireless, ath9k-devel,
netdev, linux-kernel, kernel-janitors, Peter Senna Tschudin
In-Reply-To: <1349038599-22026-1-git-send-email-peter.senna@gmail.com>
This patch remove a semicolon after if(...) that is preventing the
error check to work correctly. Removing this semicolon will change the
code behavior, but this is intended.
The semantic patch that finds this problem is as follows:
(http://coccinelle.lip6.fr/)
// <smpl>
@r1@
position p;
@@
if (...);@p
@script:python@
p0 << r1.p;
@@
// Emacs org-mode output
cocci.print_main("", p0)
cocci.print_secs("", p0)
// </smpl>
Signed-off-by: Peter Senna Tschudin <peter.senna@gmail.com>
---
NOTE: This patch changes the semantics.
Changes from V1:
Commit message
Added a note about semantics change
Changes from V2:
Commit message
drivers/net/wireless/ath/ath9k/ar9003_eeprom.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wireless/ath/ath9k/ar9003_eeprom.c b/drivers/net/wireless/ath/ath9k/ar9003_eeprom.c
index 5bbe505..189aeb2 100644
--- a/drivers/net/wireless/ath/ath9k/ar9003_eeprom.c
+++ b/drivers/net/wireless/ath/ath9k/ar9003_eeprom.c
@@ -2989,7 +2989,7 @@ static u32 ath9k_hw_ar9300_get_eeprom(struct ath_hw *ah,
case EEP_PAPRD:
if (AR_SREV_9462(ah))
return false;
- if (!ah->config.enable_paprd);
+ if (!ah->config.enable_paprd)
return false;
return !!(pBase->featureEnable & BIT(5));
case EEP_CHAIN_MASK_REDUCE:
--
1.7.11.4
^ permalink raw reply related
* Re: [PATCH 11/11] net: xilinx: Show csum in bootlog
From: Ben Hutchings @ 2012-10-10 16:14 UTC (permalink / raw)
To: Michal Simek
Cc: netdev, linux-kernel, Anirudha Sarangi, John Linn, Grant Likely,
Rob Herring, David S. Miller, John Linn
In-Reply-To: <CAHTX3dJuHxAqZOkkSymPz5ots1Edq4ty3o1rGAuhnpWrUwtWZQ@mail.gmail.com>
On Wed, 2012-10-10 at 18:06 +0200, Michal Simek wrote:
> 2012/10/9 Ben Hutchings <bhutchings@solarflare.com>:
> > On Tue, 2012-10-09 at 11:08 +0200, Michal Simek wrote:
> >> On 10/05/2012 03:51 PM, Ben Hutchings wrote:
> >> > On Fri, 2012-10-05 at 11:35 +0200, Michal Simek wrote:
> >> >> On 10/04/2012 09:15 PM, Ben Hutchings wrote:
> >> >>> On Thu, 2012-10-04 at 20:14 +0200, Michal Simek wrote:
> >> >>>> Just show current setting in bootlog.
> >> >>> [...]
> >> >>>> --- a/drivers/net/ethernet/xilinx/ll_temac_main.c
> >> >>>> +++ b/drivers/net/ethernet/xilinx/ll_temac_main.c
> >> >>>> @@ -1052,12 +1052,14 @@ static int __devinit temac_of_probe(struct platform_device *op)
> >> >>>> /* Setup checksum offload, but default to off if not specified */
> >> >>>> lp->temac_features = 0;
> >> >>>> p = (__be32 *)of_get_property(op->dev.of_node, "xlnx,txcsum", NULL);
> >> >>>> + dev_info(&op->dev, "TX_CSUM %d\n", be32_to_cpup(p));
> >> >>>> if (p && be32_to_cpu(*p)) {
> >> >>>> lp->temac_features |= TEMAC_FEATURE_TX_CSUM;
> >> >>>> /* Can checksum TCP/UDP over IPv4. */
> >> >>>> ndev->features |= NETIF_F_IP_CSUM;
> >> >>>> }
> >> >>>> p = (__be32 *)of_get_property(op->dev.of_node, "xlnx,rxcsum", NULL);
> >> >>>> + dev_info(&op->dev, "RX_CSUM %d\n", be32_to_cpup(p));
> >> >>> [...]
> >> >>>
> >> >>> Is there any particular reason you think this needs to be logged by
> >> >>> default, rather than letting users run ethtool -k? I suggest using
> >> >>> dev_dbg() instead.
> >> >>
> >> >> Ok. I have looked at it and there are missing some bits in ndev->features.
> >> >>
> >> >> Can you please check that my setting is correct?
> >> >>
> >> >> It is SG DMA ip/driver.
> >> >> ndev->features = NETIF_F_FRAGLIST | NETIF_F_SG
> >> >
> >> > NETIF_F_SG only; NETIF_F_FRAGLIST means you can handle skbs chained
> >> > through the frag_list pointer.
> >>
> >> The driver is able to handle skb fragments too. temac_start_xmit
> >
> > "git grep -w -E 'frag_list|skb_walk_frags|skb_to_sgvec' drivers/net/ethernet/xilinx"
> > returns nothing in net-next.
>
> What about this? Maybe it is different fragmentation.
> drivers/net/ethernet/xilinx/ll_temac_main.c
> 688 num_frag = skb_shinfo(skb)->nr_frags;
> 689 frag = &skb_shinfo(skb)->frags[0];
[...]
Yes, the fact that you handle the frags array is indicated by feature
flag NETIF_F_SG.
Ben.
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply
* Re: pull request: wireless 2012-10-09
From: David Miller @ 2012-10-10 16:06 UTC (permalink / raw)
To: linville; +Cc: linux-wireless, netdev, linux-kernel
In-Reply-To: <20121009183844.GB1993@tuxdriver.com>
From: "John W. Linville" <linville@tuxdriver.com>
Date: Tue, 9 Oct 2012 14:38:44 -0400
> Here is a batch of fixes intended for 3.7...
>
> Amitkumar Karwar provides a couple of mwifiex fixes to correctly
> report some reason codes for certain connection failures. He also
> provides a fix to cleanup after a scanning failure. Bing Zhao rounds
> that out with another mwifiex scanning fix.
>
> Daniel Golle gives us a fix for a copy/paste error in rt2x00.
>
> Felix Fietkau brings a couple of ath9k fixes related to suspend/resume,
> and a couple of fixes to prevent memory leaks in ath9k and mac80211.
>
> Ronald Wahl sends a carl9170 fix for a sleep in softirq context.
>
> Thomas Pedersen reorders some code to prevent drv_get_tsf from being
> called while holding a spinlock, now that it can sleep.
>
> Finally, Wei Yongjun prevents a NULL pointer dereference in the
> ath5k driver.
...
> git://git.kernel.org/pub/scm/linux/kernel/git/linville/wireless.git tags/master-2012-10-08
Pulled, thanks John.
^ permalink raw reply
* Re: [PATCH 11/11] net: xilinx: Show csum in bootlog
From: Michal Simek @ 2012-10-10 16:06 UTC (permalink / raw)
To: Ben Hutchings
Cc: netdev, linux-kernel, Anirudha Sarangi, John Linn, Grant Likely,
Rob Herring, David S. Miller, John Linn
In-Reply-To: <1349801800.2800.21.camel@bwh-desktop.uk.solarflarecom.com>
2012/10/9 Ben Hutchings <bhutchings@solarflare.com>:
> On Tue, 2012-10-09 at 11:08 +0200, Michal Simek wrote:
>> On 10/05/2012 03:51 PM, Ben Hutchings wrote:
>> > On Fri, 2012-10-05 at 11:35 +0200, Michal Simek wrote:
>> >> On 10/04/2012 09:15 PM, Ben Hutchings wrote:
>> >>> On Thu, 2012-10-04 at 20:14 +0200, Michal Simek wrote:
>> >>>> Just show current setting in bootlog.
>> >>> [...]
>> >>>> --- a/drivers/net/ethernet/xilinx/ll_temac_main.c
>> >>>> +++ b/drivers/net/ethernet/xilinx/ll_temac_main.c
>> >>>> @@ -1052,12 +1052,14 @@ static int __devinit temac_of_probe(struct platform_device *op)
>> >>>> /* Setup checksum offload, but default to off if not specified */
>> >>>> lp->temac_features = 0;
>> >>>> p = (__be32 *)of_get_property(op->dev.of_node, "xlnx,txcsum", NULL);
>> >>>> + dev_info(&op->dev, "TX_CSUM %d\n", be32_to_cpup(p));
>> >>>> if (p && be32_to_cpu(*p)) {
>> >>>> lp->temac_features |= TEMAC_FEATURE_TX_CSUM;
>> >>>> /* Can checksum TCP/UDP over IPv4. */
>> >>>> ndev->features |= NETIF_F_IP_CSUM;
>> >>>> }
>> >>>> p = (__be32 *)of_get_property(op->dev.of_node, "xlnx,rxcsum", NULL);
>> >>>> + dev_info(&op->dev, "RX_CSUM %d\n", be32_to_cpup(p));
>> >>> [...]
>> >>>
>> >>> Is there any particular reason you think this needs to be logged by
>> >>> default, rather than letting users run ethtool -k? I suggest using
>> >>> dev_dbg() instead.
>> >>
>> >> Ok. I have looked at it and there are missing some bits in ndev->features.
>> >>
>> >> Can you please check that my setting is correct?
>> >>
>> >> It is SG DMA ip/driver.
>> >> ndev->features = NETIF_F_FRAGLIST | NETIF_F_SG
>> >
>> > NETIF_F_SG only; NETIF_F_FRAGLIST means you can handle skbs chained
>> > through the frag_list pointer.
>>
>> The driver is able to handle skb fragments too. temac_start_xmit
>
> "git grep -w -E 'frag_list|skb_walk_frags|skb_to_sgvec' drivers/net/ethernet/xilinx"
> returns nothing in net-next.
What about this? Maybe it is different fragmentation.
drivers/net/ethernet/xilinx/ll_temac_main.c
688 num_frag = skb_shinfo(skb)->nr_frags;
689 frag = &skb_shinfo(skb)->frags[0];
>
> [...]
>> >> rx Full csum -> NETIF_F_RXCSUM
>> >>
>> >> Is there any option to support partial csum?
>> >
>> > There is no need to differentiate these in the device features. For TX
>> > the stack needs to know whether to use a software fallback before
>> > passing the skb to you, but on RX it looks at the ip_summed field of
>> > each skb you pass up.
>>
>> Hardware can be setup asymmetrically. It means enable CSUM only on RX or TX.
>> All combination are valid.
>> The point here is if Linux is not able to handle this then we have to
>> create logic in the driver to support these options too.
>
> Linux handles this just fine. The point is you don't have to tell the
> stack in advance whether or what kind of RX checksum validation your
> devices will do. (In fact the only reason that feature flag exists at
> all is so that it can be generically exposed through the ethtool API.)
Ok.
Thanks,
Michal
--
Michal Simek, Ing. (M.Eng)
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel 2.6 Microblaze Linux - http://www.monstr.eu/fdt/
Microblaze U-BOOT custodian
^ permalink raw reply
* Re: [patch] isdn: fix a wrapping bug in isdn_ppp_ioctl()
From: Joe Perches @ 2012-10-10 15:52 UTC (permalink / raw)
To: David Laight
Cc: Andreas Schwab, Dan Carpenter, Karsten Keil, David S. Miller,
Masanari Iida, netdev, kernel-janitors
In-Reply-To: <AE90C24D6B3A694183C094C60CF0A2F6026B7034@saturn3.aculab.com>
On Wed, 2012-10-10 at 16:44 +0100, David Laight wrote:
> > On Wed, 2012-10-10 at 15:59 +0100, David Laight wrote:
> > > Seems to me the code is expecting 256 bits of data, not any multiple of int,
> > > long or anything else.
> >
> > include/linux/isdn_ppp.h:#define PPPIOCGCOMPRESSORS _IOR('t',134,unsigned long [8])
>
> That doesn't mean the whole thing makes any sense on 64bit systems.
> A whole load of historic code used 'long' to ensure 32bit.
> Some of that might have crept into Linux sources.
Very true, but it's exported via copy_to_user.
> Since I suspect there are a maximum of 256 bits on both 32 and 64bit
> systems, I wouldn't like to guess exactly how any particular 64bit
> application chooses to check the bitmap.
>
> The ioctl constant may be wrong on 64 bit systems.
shrug. Not much to do about it now.
isdn isn't very active.
Karsten? What do you think?
^ permalink raw reply
* RE: [patch] isdn: fix a wrapping bug in isdn_ppp_ioctl()
From: David Laight @ 2012-10-10 15:44 UTC (permalink / raw)
To: Joe Perches
Cc: Andreas Schwab, Dan Carpenter, Karsten Keil, David S. Miller,
Masanari Iida, netdev, kernel-janitors
In-Reply-To: <1349882347.2386.47.camel@joe-AO722>
> On Wed, 2012-10-10 at 15:59 +0100, David Laight wrote:
> > Seems to me the code is expecting 256 bits of data, not any multiple of int,
> > long or anything else.
>
> include/linux/isdn_ppp.h:#define PPPIOCGCOMPRESSORS _IOR('t',134,unsigned long [8])
That doesn't mean the whole thing makes any sense on 64bit systems.
A whole load of historic code used 'long' to ensure 32bit.
Some of that might have crept into Linux sources.
Since I suspect there are a maximum of 256 bits on both 32 and 64bit
systems, I wouldn't like to guess exactly how any particular 64bit
application chooses to check the bitmap.
The ioctl constant may be wrong on 64 bit systems.
David
^ permalink raw reply
* Re: [patch] isdn: fix a wrapping bug in isdn_ppp_ioctl()
From: Joe Perches @ 2012-10-10 15:19 UTC (permalink / raw)
To: David Laight
Cc: Andreas Schwab, Dan Carpenter, Karsten Keil, David S. Miller,
Masanari Iida, netdev, kernel-janitors
In-Reply-To: <AE90C24D6B3A694183C094C60CF0A2F6026B7033@saturn3.aculab.com>
On Wed, 2012-10-10 at 15:59 +0100, David Laight wrote:
> Seems to me the code is expecting 256 bits of data, not any multiple of int,
> long or anything else.
include/linux/isdn_ppp.h:#define PPPIOCGCOMPRESSORS _IOR('t',134,unsigned long [8])
^ permalink raw reply
* RE: [patch] isdn: fix a wrapping bug in isdn_ppp_ioctl()
From: David Laight @ 2012-10-10 14:59 UTC (permalink / raw)
To: Joe Perches, Andreas Schwab
Cc: Dan Carpenter, Karsten Keil, David S. Miller, Masanari Iida,
netdev, kernel-janitors
In-Reply-To: <1349880119.2386.46.camel@joe-AO722>
> -----Original Message-----
> From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org] On Behalf Of Joe Perches
> Sent: 10 October 2012 15:42
> To: Andreas Schwab
> Cc: Dan Carpenter; Karsten Keil; David S. Miller; Masanari Iida; netdev@vger.kernel.org; kernel-
> janitors@vger.kernel.org
> Subject: Re: [patch] isdn: fix a wrapping bug in isdn_ppp_ioctl()
>
> On Wed, 2012-10-10 at 15:58 +0200, Andreas Schwab wrote:
> > Sorry, I was misremembering the history of the bit ops. There has
> > historically been issues with varying bit order, but noadays set_bit is
> > always defined consistently with C shifts.
>
> No worries. Anyway, the change was suggested to aid
> reader comprehension. If it doesn't (and it seems not)
> then it's not worth it.
>
> Anyway, there is still the open question of an overrun/info
> leak.
>
> > > - if ((r = set_arg(argp, protos, 8 * sizeof(long))))
>
> set_arg's 2nd arg is bytes not bits.
Seems to me the code is expecting 256 bits of data, not any multiple of int,
long or anything else.
David
^ permalink raw reply
* Re: [Xen-devel] compound skb frag pages appearing in start_xmit
From: Sander Eikelenboom @ 2012-10-10 14:49 UTC (permalink / raw)
To: Ian Campbell
Cc: Eric Dumazet, netdev@vger.kernel.org, Eric Dumazet, xen-devel,
Konrad Rzeszutek Wilk
In-Reply-To: <1349874598.10070.39.camel@zakaz.uk.xensource.com>
Wednesday, October 10, 2012, 3:09:58 PM, you wrote:
> On Wed, 2012-10-10 at 11:13 +0100, Ian Campbell wrote:
>> I haven't tackled netfront yet.
> I seem to be totally unable to reproduce the equivalent issue on the
> netfront xmit side, even though it seems like the loop in
> xennet_make_frags ought to be obviously susceptible to it.
> Konrad, Sander, are either of you able to repro, e.g. with:
Hmrrrmm i don't see any traces, only strange behaviour ..
- i can connect to guests by ssh, but it's sluggish, and sometimes stops working
- The guest seem to keep trying to connect to netback:
[ 658.276719] xen_bridge: port 2(vif40.0) entered forwarding state
[ 658.282258] xen_bridge: port 2(vif40.0) entered forwarding state
[ 663.945964] xen_bridge: port 7(vif39.0) entered forwarding state
[ 669.674277] xen_bridge: port 2(vif40.0) entered disabled state
[ 669.680290] device vif40.0 left promiscuous mode
[ 669.685464] xen_bridge: port 2(vif40.0) entered disabled state
[ 672.857222] device vif41.0 entered promiscuous mode
[ 673.166254] xen-blkback:ring-ref 8, event-channel 9, protocol 1 (x86_64-abi)
[ 673.176368] xen_bridge: port 2(vif41.0) entered forwarding state
[ 673.182042] xen_bridge: port 2(vif41.0) entered forwarding state
[ 674.439725] xen_bridge: port 7(vif39.0) entered disabled state
[ 674.445708] device vif39.0 left promiscuous mode
[ 674.450955] xen_bridge: port 7(vif39.0) entered disabled state
[ 677.726040] device vif42.0 entered promiscuous mode
[ 678.053381] xen-blkback:ring-ref 8, event-channel 9, protocol 1 (x86_64-abi)
[ 678.062804] xen_bridge: port 7(vif42.0) entered forwarding state
[ 678.068433] xen_bridge: port 7(vif42.0) entered forwarding state
[ 688.224736] xen_bridge: port 2(vif41.0) entered forwarding state
[ 693.080557] xen_bridge: port 7(vif42.0) entered forwarding state
[ 700.786276] xen_bridge: port 7(vif42.0) entered disabled state
[ 700.792484] device vif42.0 left promiscuous mode
[ 700.802409] xen_bridge: port 7(vif42.0) entered disabled state
[ 704.133606] device vif43.0 entered promiscuous mode
[ 704.460160] xen-blkback:ring-ref 8, event-channel 9, protocol 1 (x86_64-abi)
[ 704.469800] xen_bridge: port 7(vif43.0) entered forwarding state
[ 704.475303] xen_bridge: port 7(vif43.0) entered forwarding state
[ 719.493788] xen_bridge: port 7(vif43.0) entered forwarding state
[ 726.302456] xen_bridge: port 7(vif43.0) entered disabled state
[ 726.308898] device vif43.0 left promiscuous mode
[ 726.314029] xen_bridge: port 7(vif43.0) entered disabled state
All the guests are already up, but this keeps on going and going and going ....
> diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c
> index b06ef81..8a3f770 100644
> --- a/drivers/net/xen-netfront.c
> +++ b/drivers/net/xen-netfront.c
> @@ -462,6 +462,8 @@ static void xennet_make_frags(struct sk_buff *skb, struct net_device *dev,
> ref = gnttab_claim_grant_reference(&np->gref_tx_head);
> BUG_ON((signed short)ref < 0);
>
> + BUG_ON(PageCompound(skb_frag_page(frag)));
> +
> mfn = pfn_to_mfn(page_to_pfn(skb_frag_page(frag)));
> gnttab_grant_foreign_access_ref(ref, np->xbdev->otherend_id,
> mfn, GNTMAP_readonly);
> My repro for netback was just to netcat a wodge of data from dom0->domU
> but going the other way doesn't seem to trigger.
^ permalink raw reply
* Re: [patch] isdn: fix a wrapping bug in isdn_ppp_ioctl()
From: Joe Perches @ 2012-10-10 14:41 UTC (permalink / raw)
To: Andreas Schwab
Cc: Dan Carpenter, Karsten Keil, David S. Miller, Masanari Iida,
netdev, kernel-janitors
In-Reply-To: <m2sj9m5sz5.fsf@igel.home>
On Wed, 2012-10-10 at 15:58 +0200, Andreas Schwab wrote:
> Sorry, I was misremembering the history of the bit ops. There has
> historically been issues with varying bit order, but noadays set_bit is
> always defined consistently with C shifts.
No worries. Anyway, the change was suggested to aid
reader comprehension. If it doesn't (and it seems not)
then it's not worth it.
Anyway, there is still the open question of an overrun/info
leak.
> > - if ((r = set_arg(argp, protos, 8 * sizeof(long))))
set_arg's 2nd arg is bytes not bits.
^ permalink raw reply
* Re: setting flow spec rules under vswitch configuration
From: Or Gerlitz @ 2012-10-10 14:31 UTC (permalink / raw)
To: Ben Hutchings; +Cc: Rony Efraim, netdev, Amir Vadai
In-Reply-To: <1349879306.6336.40.camel@deadeye.wl.decadent.org.uk>
On 10/10/2012 16:28, Ben Hutchings wrote:
> On Wed, 2012-10-10 at 16:16 +0200, Or Gerlitz wrote:
> [...]
>> >Thanks for setting the sketch of a plan here... so if we go little bit
>> >into details, we can safely move 20 bytes from the hadata[60] field into
>> >the beginning of struct ethtool_flow_ext, which will still allow old
>> >user space to work with newer kernels. As for newer uses space that
>> >would like to set mac addresses within
>> >ethtool_flow_ext, how are they supposed to identify if the kernel
>> >supports this extension (of the extension...)?
> Well they can't tell in advance, but we can define another flag in
> flow_type like FLOW_EXT and existing drivers will reject flow specs with
> that flag set.
I understand what we can add a flag which will be rejected by driver's
that don't support this ext,
can the same/similar flag be used for newer user space code to mark they
want this ext?
Or.
^ permalink raw reply
* Re: setting flow spec rules under vswitch configuration
From: Ben Hutchings @ 2012-10-10 14:28 UTC (permalink / raw)
To: Or Gerlitz; +Cc: Rony Efraim, netdev, Amir Vadai
In-Reply-To: <50758353.6050309@mellanox.com>
On Wed, 2012-10-10 at 16:16 +0200, Or Gerlitz wrote:
[...]
> Thanks for setting the sketch of a plan here... so if we go little bit
> into details, we can safely move 20 bytes from the hadata[60] field into
> the beginning of struct ethtool_flow_ext, which will still allow old
> user space to work with newer kernels. As for newer uses space that
> would like to set mac addresses within
> ethtool_flow_ext, how are they supposed to identify if the kernel
> supports this extension (of the extension...)?
Well they can't tell in advance, but we can define another flag in
flow_type like FLOW_EXT and existing drivers will reject flow specs with
that flag set.
> this might be newbee
> question, I didn't made many ethtool patches so far.
>
> Also on a related note, what does the 64bit data field of
> ethtool_flow_ext used for?
Driver-dependent. ixgbe uses it for something.
Ben.
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply
* Re: setting flow spec rules under vswitch configuration
From: Or Gerlitz @ 2012-10-10 14:16 UTC (permalink / raw)
To: Ben Hutchings; +Cc: Rony Efraim, netdev, Amir Vadai
In-Reply-To: <1349801311.2800.17.camel@bwh-desktop.uk.solarflarecom.com>
On 09/10/2012 18:48, Ben Hutchings wrote:
> On Tue, 2012-10-09 at 11:37 +0200, Or Gerlitz wrote:
>> Hi Ben,
>>
>> Looking on kernel ethtool flow steering APIs in the context of a device
>> which is used as the uplink of a virtual switch, the admin should be able
>> to provide flow specification and action (e.g drop) that relates to traffic
>> coming from a specific port of the switch e.g that relates to a certain
>> VM,etc.
>>
>> For that end, we need to be able to specify both the L3/L4 attributes of
>> the flow and an L2 spec, that is the L2 spec containing the destination MAC
>> can't be assumed as the one of that device.
>>
>> Specifically, in struct ethtool_rx_ntuple_flow_spec, I think we should
>> let the
>> to provide an ethhdr even when L3/L4 spec is given, make sense?
> Yes, but the ethertype looks redundant - the inner type is implied by
> the L3 flow type and the outer type for a VLAN-encapsulated packet
> should be matched against ethtool_flow_ext::vlan_etype. Might be better
> to avoid confusion by just specifying the L2 addresses.
>
>> if yes, how
>> would you like to see this change, add a union entry that contains both,
>> or in
>> another way?
> struct ethtool_rx_ntuple_flow_spec is obsolete; struct
> ethtool_rx_flow_spec is what we have to consider. That effectively has:
>
> union ethtool_flow_union {
> struct ethtool_tcpip4_spec tcp_ip4_spec;
> struct ethtool_tcpip4_spec udp_ip4_spec;
> struct ethtool_tcpip4_spec sctp_ip4_spec;
> struct ethtool_ah_espip4_spec ah_ip4_spec;
> struct ethtool_ah_espip4_spec esp_ip4_spec;
> struct ethtool_usrip4_spec usr_ip4_spec;
> struct ethhdr ether_spec;
> /* above are up to 16 bytes long */
> __u8 hdata[60];
> } h_u;
> struct ethtool_flow_ext {
> __be16 vlan_etype;
> __be16 vlan_tci;
> __be32 data[2];
> } h_ext;
> union ethtool_flow_union m_u;
> struct ethtool_flow_ext m_ext;
>
> So ethtool_flow_union::hdata currently provides 44 bytes of padding
> between the per-protocol flow specs and ethtool_flow_ext, which can be
> reallocated to the *beginning* of ethtool_flow_ext. At some point we'll
> presumably want to add IPv6 flow specs, which will use up 24 bytes of
> that padding at the front. So we can potentially extend
> ethtool_flow_ext by up to 20 bytes.
>
> Ben.
>
Ben,
Thanks for setting the sketch of a plan here... so if we go little bit
into details, we can safely move 20 bytes from the hadata[60] field into
the beginning of struct ethtool_flow_ext, which will still allow old
user space to work with newer kernels. As for newer uses space that
would like to set mac addresses within
ethtool_flow_ext, how are they supposed to identify if the kernel
supports this extension (of the extension...)? this might be newbee
question, I didn't made many ethtool patches so far.
Also on a related note, what does the 64bit data field of
ethtool_flow_ext used for?
Or.
^ permalink raw reply
* Re: [PATCH V1 3/3] net/mlx4_en: Add HW timestamping (TS) support
From: Or Gerlitz @ 2012-10-10 14:06 UTC (permalink / raw)
To: Richard Cochran; +Cc: davem, netdev, Eugenia Emantayev, Tzahi Oved
In-Reply-To: <20121010033703.GA2304@netboy.at.omicron.at>
On 10/10/2012 05:37, Richard Cochran wrote:
> On Tue, Oct 09, 2012 at 05:20:13PM +0200, Or Gerlitz wrote:
>
>> @@ -1542,6 +1611,7 @@ static const struct net_device_ops mlx4_netdev_ops = {
>> .ndo_set_mac_address = mlx4_en_set_mac,
>> .ndo_validate_addr = eth_validate_addr,
>> .ndo_change_mtu = mlx4_en_change_mtu,
>> + .ndo_do_ioctl = mlx4_en_ioctl,
>> .ndo_tx_timeout = mlx4_en_tx_timeout,
>> .ndo_vlan_rx_add_vid = mlx4_en_vlan_rx_add_vid,
>> .ndo_vlan_rx_kill_vid = mlx4_en_vlan_rx_kill_vid,
> You should also provide a get_ts_info function to let the users know
> about the device's time stamping capabilities.
sure, will do.
>
>> diff --git a/drivers/net/ethernet/mellanox/mlx4/en_timestamp.c b/drivers/net/ethernet/mellanox
>> ...
>>
>> +void mlx4_en_init_timestamp(struct mlx4_en_dev *mdev)
>> +{
>> + struct mlx4_dev *dev = mdev->dev;
>> + u64 temp_mult;
>> +
>> + memset(&mdev->cycles, 0, sizeof(mdev->cycles));
>> + mdev->cycles.read = mlx4_en_read_clock;
>> + mdev->cycles.mask = CLOCKSOURCE_MASK(48);
>> +
>> + /* we have hca_core_clock in MHz, so to translate cycles to nsecs
>> + * we need to divide cycles by freq and multiply by 1000;
>> + * in order to get precise result we shift left the value,
>> + * since we don't have floating point there;
>> + * at the end shift result back
>> + */
>> + temp_mult = ((1ull * 1000) << 29) / dev->caps.hca_core_clock;
>> + mdev->cycles.mult = (u32)temp_mult;
>> + mdev->cycles.shift = 29;
>> +
>> + timecounter_init(&mdev->clock, &mdev->cycles,
>> + ktime_to_ns(ktime_get_real()));
> I didn't see any watchdog code to read this clock periodically, in order to catch overflows.
Thanks for pointing this out, will look on that and see what needs to be
fixed for V2
Or.
^ permalink raw reply
* Re: [patch] isdn: fix a wrapping bug in isdn_ppp_ioctl()
From: Andreas Schwab @ 2012-10-10 13:58 UTC (permalink / raw)
To: Joe Perches
Cc: Dan Carpenter, Karsten Keil, David S. Miller, Masanari Iida,
netdev, kernel-janitors
In-Reply-To: <1349874007.2386.42.camel@joe-AO722>
Sorry, I was misremembering the history of the bit ops. There has
historically been issues with varying bit order, but noadays set_bit is
always defined consistently with C shifts.
But another issue, set_bit is an atomic operation which may be
significantly more expensive than the equivalent C operation. Better
use __set_bit when atomicity is not needed.
Andreas.
--
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5
"And now for something completely different."
^ permalink raw reply
* [PATCH V2] xen: netback: handle compound page fragments on transmit.
From: Ian Campbell @ 2012-10-10 13:48 UTC (permalink / raw)
To: netdev, xen-devel
Cc: Ian Campbell, Eric Dumazet, Konrad Rzeszutek Wilk,
Sander Eikelenboom
An SKB paged fragment can consist of a compound page with order > 0.
However the netchannel protocol deals only in PAGE_SIZE frames.
Handle this in netbk_gop_frag_copy and xen_netbk_count_skb_slots by
iterating over the frames which make up the page.
Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Cc: Eric Dumazet <eric.dumazet@gmail.com>
Cc: Konrad Rzeszutek Wilk <konrad@kernel.org>
Cc: Sander Eikelenboom <linux@eikelenboom.it>
---
v2: Only move to next frame if there is data remaining.
---
drivers/net/xen-netback/netback.c | 40 ++++++++++++++++++++++++++++++++----
1 files changed, 35 insertions(+), 5 deletions(-)
diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
index 4ebfcf3..f2d6b78 100644
--- a/drivers/net/xen-netback/netback.c
+++ b/drivers/net/xen-netback/netback.c
@@ -335,21 +335,35 @@ unsigned int xen_netbk_count_skb_slots(struct xenvif *vif, struct sk_buff *skb)
for (i = 0; i < skb_shinfo(skb)->nr_frags; i++) {
unsigned long size = skb_frag_size(&skb_shinfo(skb)->frags[i]);
+ unsigned long offset = skb_shinfo(skb)->frags[i].page_offset;
unsigned long bytes;
+
+ offset &= ~PAGE_MASK;
+
while (size > 0) {
+ BUG_ON(offset >= PAGE_SIZE);
BUG_ON(copy_off > MAX_BUFFER_OFFSET);
- if (start_new_rx_buffer(copy_off, size, 0)) {
+ bytes = PAGE_SIZE - offset;
+
+ if (bytes > size)
+ bytes = size;
+
+ if (start_new_rx_buffer(copy_off, bytes, 0)) {
count++;
copy_off = 0;
}
- bytes = size;
if (copy_off + bytes > MAX_BUFFER_OFFSET)
bytes = MAX_BUFFER_OFFSET - copy_off;
copy_off += bytes;
+
+ offset += bytes;
size -= bytes;
+
+ if (offset == PAGE_SIZE)
+ offset = 0;
}
}
return count;
@@ -403,14 +417,24 @@ static void netbk_gop_frag_copy(struct xenvif *vif, struct sk_buff *skb,
unsigned long bytes;
/* Data must not cross a page boundary. */
- BUG_ON(size + offset > PAGE_SIZE);
+ BUG_ON(size + offset > PAGE_SIZE<<compound_order(page));
meta = npo->meta + npo->meta_prod - 1;
+ /* Skip unused frames from start of page */
+ page += offset >> PAGE_SHIFT;
+ offset &= ~PAGE_MASK;
+
while (size > 0) {
+ BUG_ON(offset >= PAGE_SIZE);
BUG_ON(npo->copy_off > MAX_BUFFER_OFFSET);
- if (start_new_rx_buffer(npo->copy_off, size, *head)) {
+ bytes = PAGE_SIZE - offset;
+
+ if (bytes > size)
+ bytes = size;
+
+ if (start_new_rx_buffer(npo->copy_off, bytes, *head)) {
/*
* Netfront requires there to be some data in the head
* buffer.
@@ -420,7 +444,6 @@ static void netbk_gop_frag_copy(struct xenvif *vif, struct sk_buff *skb,
meta = get_next_rx_buffer(vif, npo);
}
- bytes = size;
if (npo->copy_off + bytes > MAX_BUFFER_OFFSET)
bytes = MAX_BUFFER_OFFSET - npo->copy_off;
@@ -453,6 +476,13 @@ static void netbk_gop_frag_copy(struct xenvif *vif, struct sk_buff *skb,
offset += bytes;
size -= bytes;
+ /* Next frame */
+ if (offset == PAGE_SIZE && size) {
+ BUG_ON(!PageCompound(page));
+ page++;
+ offset = 0;
+ }
+
/* Leave a gap for the GSO descriptor. */
if (*head && skb_shinfo(skb)->gso_size && !vif->gso_prefix)
vif->rx.req_cons++;
--
1.7.2.5
^ permalink raw reply related
* Re: [Xen-devel] compound skb frag pages appearing in start_xmit
From: Sander Eikelenboom @ 2012-10-10 13:31 UTC (permalink / raw)
To: Ian Campbell
Cc: Eric Dumazet, netdev@vger.kernel.org, Eric Dumazet,
Konrad Rzeszutek Wilk, xen-devel
In-Reply-To: <1349872149.10070.31.camel@zakaz.uk.xensource.com>
Wednesday, October 10, 2012, 2:29:09 PM, you wrote:
> On Wed, 2012-10-10 at 13:24 +0100, Sander Eikelenboom wrote:
>> Wednesday, October 10, 2012, 12:13:04 PM, you wrote:
>>
>> > On Tue, 2012-10-09 at 15:40 +0100, Ian Campbell wrote:
>> >> On Tue, 2012-10-09 at 15:27 +0100, Eric Dumazet wrote:
>> >> > On Tue, 2012-10-09 at 15:17 +0100, Ian Campbell wrote:
>> >> >
>> >> > > Does the higher order pages effectively reduce the number of frags which
>> >> > > are in use? e.g if MAX_SKB_FRAGS is 16, then for order-0 pages you could
>> >> > > have 64K worth of frag data.
>> >> > >
>> >> > > If we switch to order-3 pages everywhere then can the skb contain 512K
>> >> > > of data, or does the effective maximum number of frags in an skb reduce
>> >> > > to 2?
>> >> >
>> >> > effective number of frags reduce to 2 or 3
>> >> >
>> >> > (We still limit GSO packets to ~63536 bytes)
>> >>
>> >> Great! Then I think the fix is more/less trivial...
>>
>> > The following seems to work for me.
>>
>> But it doesn't seem to work for me ... dmesg attached.
>> [ 191.777994] ------------[ cut here ]------------
>> [ 191.784245] kernel BUG at drivers/net/xen-netback/netback.c:481!
> Looks like that BUG_ON is a little aggressive. It'll trigger if the data
> happens to end on a frame boundary. Hopefully this will fix it for you:
Yes it does !
Thanks .. will recompile and test the netfront case as well
--
Sander
> diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
> index d747e30..f2d6b78 100644
> --- a/drivers/net/xen-netback/netback.c
> +++ b/drivers/net/xen-netback/netback.c
> @@ -477,7 +477,7 @@ static void netbk_gop_frag_copy(struct xenvif *vif, struct sk_buff *skb,
> size -= bytes;
>
> /* Next frame */
> - if (offset == PAGE_SIZE) {
> + if (offset == PAGE_SIZE && size) {
> BUG_ON(!PageCompound(page));
> page++;
> offset = 0;
^ permalink raw reply
* Re: [RFC PATCH] net/core: support runtime PM on net_device
From: Oliver Neukum @ 2012-10-10 13:17 UTC (permalink / raw)
To: Ming Lei; +Cc: David S. Miller, Rafael J. Wysocki, Alan Stern, netdev, linux-pm
In-Reply-To: <1349873913-10701-1-git-send-email-ming.lei@canonical.com>
On Wednesday 10 October 2012 20:58:33 Ming Lei wrote:
> In ioctl path on net_device, the physical deivce is often
> touched, but the physical device may have been put into runtime
> suspend state already, so cause some utilitis(ifconfig, ethtool,
> ...) to return failure in this situation.
>
> This patch enables runtime PM on net_device and mark it as
> no_callbacks, and resumes the net_device if physical device
> is to be accessed, then suspends it after completion of the
> access.
>
> This patch fixes the problem above.
>
[..]
> + if (pm_runtime_get_sync(&dev->dev) < 0)
> + return -ENODEV;
-EIO would be appropriate.
> + err = __dev_ifsioc(net, ifr, cmd);
> +
> + pm_runtime_put(&dev->dev);
> +
> + return err;
> +}
> +
> /*
> * This function handles all "interface"-type I/O control requests. The actual
> * 'doing' part of this is dev_ifsioc above.
> diff --git a/net/core/ethtool.c b/net/core/ethtool.c
> index 4d64cc2..2dc43da 100644
> --- a/net/core/ethtool.c
> +++ b/net/core/ethtool.c
> @@ -25,6 +25,7 @@
> #include <linux/slab.h>
> #include <linux/rtnetlink.h>
> #include <linux/sched.h>
> +#include <linux/pm_runtime.h>
>
> /*
> * Some useful ethtool_ops methods that're device independent.
> @@ -1464,10 +1465,13 @@ int dev_ethtool(struct net *net, struct ifreq *ifr)
> return -EPERM;
> }
>
> + if ((ret = pm_runtime_get_sync(&dev->dev)) < 0)
> + return -ENODEV;
> +
> if (dev->ethtool_ops->begin) {
Perhaps you should check that first.
> rc = dev->ethtool_ops->begin(dev);
> if (rc < 0)
> - return rc;
> + goto exit;
> }
> old_features = dev->features;
>
> @@ -1648,6 +1652,7 @@ int dev_ethtool(struct net *net, struct ifreq *ifr)
>
> if (old_features != dev->features)
> netdev_features_change(dev);
> -
> +exit:
> + pm_runtime_put(&dev->dev);
> return rc;
> }
> diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
> index bcf02f6..c9adb89 100644
> --- a/net/core/net-sysfs.c
> +++ b/net/core/net-sysfs.c
> @@ -23,6 +23,7 @@
> #include <linux/export.h>
> #include <linux/jiffies.h>
> #include <net/wext.h>
> +#include <linux/pm_runtime.h>
>
> #include "net-sysfs.h"
>
> @@ -1415,6 +1416,9 @@ int netdev_register_kobject(struct net_device *net)
> if (error)
> return error;
>
> + pm_runtime_no_callbacks(dev);
> + pm_runtime_enable(dev);
Why?
Regards
Oliver
^ permalink raw reply
* Re: [Xen-devel] compound skb frag pages appearing in start_xmit
From: Ian Campbell @ 2012-10-10 13:09 UTC (permalink / raw)
To: Eric Dumazet
Cc: netdev@vger.kernel.org, Eric Dumazet, xen-devel,
Konrad Rzeszutek Wilk, Sander Eikelenboom
In-Reply-To: <1349863984.10070.26.camel@zakaz.uk.xensource.com>
On Wed, 2012-10-10 at 11:13 +0100, Ian Campbell wrote:
> I haven't tackled netfront yet.
I seem to be totally unable to reproduce the equivalent issue on the
netfront xmit side, even though it seems like the loop in
xennet_make_frags ought to be obviously susceptible to it.
Konrad, Sander, are either of you able to repro, e.g. with:
diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c
index b06ef81..8a3f770 100644
--- a/drivers/net/xen-netfront.c
+++ b/drivers/net/xen-netfront.c
@@ -462,6 +462,8 @@ static void xennet_make_frags(struct sk_buff *skb, struct net_device *dev,
ref = gnttab_claim_grant_reference(&np->gref_tx_head);
BUG_ON((signed short)ref < 0);
+ BUG_ON(PageCompound(skb_frag_page(frag)));
+
mfn = pfn_to_mfn(page_to_pfn(skb_frag_page(frag)));
gnttab_grant_foreign_access_ref(ref, np->xbdev->otherend_id,
mfn, GNTMAP_readonly);
My repro for netback was just to netcat a wodge of data from dom0->domU
but going the other way doesn't seem to trigger.
^ permalink raw reply related
* Re: [patch] isdn: fix a wrapping bug in isdn_ppp_ioctl()
From: Joe Perches @ 2012-10-10 13:00 UTC (permalink / raw)
To: Andreas Schwab
Cc: Dan Carpenter, Karsten Keil, David S. Miller, Masanari Iida,
netdev, kernel-janitors
In-Reply-To: <m21uh67csy.fsf@igel.home>
On Wed, 2012-10-10 at 14:05 +0200, Andreas Schwab wrote:
> Joe Perches <joe@perches.com> writes:
>
> > @@ -589,16 +589,15 @@ isdn_ppp_ioctl(int min, struct file *file, unsigned int cmd, unsigned long arg)
> > break;
> > case PPPIOCGCOMPRESSORS:
> > {
> > - unsigned long protos[8] = {0,};
> > + DECLARE_BITMAP(protos, BITS_PER_LONG * 8) = { 0, };
> > struct isdn_ppp_compressor *ipc = ipc_head;
> > +
> > while (ipc) {
> > - j = ipc->num / (sizeof(long) * 8);
> > - i = ipc->num % (sizeof(long) * 8);
> > - if (j < 8)
> > - protos[j] |= (0x1 << i);
> > + if (ipc->num < BITS_PER_LONG * 8)
> > + set_bit(ipc->num, protos);
> > ipc = ipc->next;
> > }
> > - if ((r = set_arg(argp, protos, 8 * sizeof(long))))
> > + if ((r = set_arg(argp, protos, sizeof(protos))))
>
> This changes the bit endianess.
How does it do that?
include/linux/bitops.h:#define BIT_MASK(nr) (1UL << ((nr) % BITS_PER_LONG))
include/linux/bitops.h:#define BIT_WORD(nr) ((nr) / BITS_PER_LONG)
include/asm-generic/bitops/atomic.h:static inline void set_bit(int nr, volatile unsigned long *addr)
include/asm-generic/bitops/atomic.h-{
include/asm-generic/bitops/atomic.h- unsigned long mask = BIT_MASK(nr);
include/asm-generic/bitops/atomic.h- unsigned long *p = ((unsigned long *)addr) + BIT_WORD(nr);
include/asm-generic/bitops/atomic.h- unsigned long flags;
include/asm-generic/bitops/atomic.h-
include/asm-generic/bitops/atomic.h- _atomic_spin_lock_irqsave(p, flags);
include/asm-generic/bitops/atomic.h- *p |= mask;
include/asm-generic/bitops/atomic.h- _atomic_spin_unlock_irqrestore(p, flags);
include/asm-generic/bitops/atomic.h-}
^ permalink raw reply
* [PATCH] atm: he: use module_pci_driver to simplify the code
From: Wei Yongjun @ 2012-10-10 13:00 UTC (permalink / raw)
To: chas; +Cc: yongjun_wei, linux-atm-general, netdev
From: Wei Yongjun <yongjun_wei@trendmicro.com.cn>
Use the module_pci_driver() macro to make the code simpler
by eliminating module_init and module_exit calls.
dpatch engine is used to auto generate this patch.
(https://github.com/weiyj/dpatch)
Signed-off-by: Wei Yongjun <yongjun_wei@trendmicro.com.cn>
---
drivers/atm/he.c | 13 +------------
1 file changed, 1 insertion(+), 12 deletions(-)
diff --git a/drivers/atm/he.c b/drivers/atm/he.c
index b182c2f..e184339 100644
--- a/drivers/atm/he.c
+++ b/drivers/atm/he.c
@@ -2883,15 +2883,4 @@ static struct pci_driver he_driver = {
.id_table = he_pci_tbl,
};
-static int __init he_init(void)
-{
- return pci_register_driver(&he_driver);
-}
-
-static void __exit he_cleanup(void)
-{
- pci_unregister_driver(&he_driver);
-}
-
-module_init(he_init);
-module_exit(he_cleanup);
+module_pci_driver(he_driver);
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox