From: "Robert Dobrowolski" <robert.dobrowolski@linux.intel.com>
To: "Bjørn Mork" <bjorn@mork.no>
Cc: "Robert Dobrowolski" <robert.dobrowolski@linux.intel.com>,
linux-usb@vger.kernel.org, rafal.f.redzimski@intel.com,
stable@vger.kernel.org, oliver@neukum.org,
netdev@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v1] net: cdc_ncm: update datagram size after changing mtu
Date: Mon, 11 Apr 2016 05:00:36 -0700 (PDT) [thread overview]
Message-ID: <52026.10.237.143.103.1460376036.squirrel@linux.intel.com> (raw)
In-Reply-To: <87h9fex0ql.fsf@nemi.mork.no>
> Robert Dobrowolski <robert.dobrowolski@linux.intel.com> writes:
>
>> From: Rafal Redzimski <rafal.f.redzimski@intel.com>
>>
>> Current implementation updates the mtu size and notify cdc_ncm
>> device using USB_CDC_SET_MAX_DATAGRAM_SIZE request about datagram
>> size change instead of changing rx_urb_size.
>>
>> Whenever mtu is being changed, datagram size should also be
>> updated.
>
> Definitely! Thanks for this. But looking at the code I believe you
> need to fix the calculation of maxmtu too. It is currently:
>
> int maxmtu = ctx->max_datagram_size - cdc_ncm_eth_hlen(dev);
>
> And cdc_ncm_set_dgram_size() updates ctx->max_datagram_size with the new
> mtu, meaning that you can only reduce the mtu. We should probably use
> cdc_ncm_max_dgram_size() instead here.
>
> And cdc_ncm_set_dgram_size() takes the datagram size with header as
> input (ref the above maxmtu calucalution), so it probably needs to
> called as
>
> cdc_ncm_set_dgram_size(dev, new_mtu + cdc_ncm_eth_hlen(dev));
>
> to get it right. I think. None of this is tested on an actual device
> yet... Care to test if I'm right, and do a v2 if necessry?
>
>> Cc: <stable@vger.kernel.org>
>
> This should be dropped for net. Ask David to queue it for stable
> instead. I usually do that by using a subject prefix like
>
> [PATCH net,stable v1] ...
>
>
>
>
> Bjørn
>
ok, thanks for feedback I will send v2 patch
WARNING: multiple messages have this Message-ID (diff)
From: "Robert Dobrowolski" <robert.dobrowolski@linux.intel.com>
To: "Bjørn Mork" <bjorn@mork.no>
Cc: "Robert Dobrowolski" <robert.dobrowolski@linux.intel.com>,
linux-usb@vger.kernel.org, rafal.f.redzimski@intel.com,
stable@vger.kernel.org, oliver@neukum.org,
netdev@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v1] net: cdc_ncm: update datagram size after changing mtu
Date: Mon, 11 Apr 2016 05:00:36 -0700 (PDT) [thread overview]
Message-ID: <52026.10.237.143.103.1460376036.squirrel@linux.intel.com> (raw)
In-Reply-To: <87h9fex0ql.fsf@nemi.mork.no>
> Robert Dobrowolski <robert.dobrowolski@linux.intel.com> writes:
>
>> From: Rafal Redzimski <rafal.f.redzimski@intel.com>
>>
>> Current implementation updates the mtu size and notify cdc_ncm
>> device using USB_CDC_SET_MAX_DATAGRAM_SIZE request about datagram
>> size change instead of changing rx_urb_size.
>>
>> Whenever mtu is being changed, datagram size should also be
>> updated.
>
> Definitely! Thanks for this. But looking at the code I believe you
> need to fix the calculation of maxmtu too. It is currently:
>
> int maxmtu = ctx->max_datagram_size - cdc_ncm_eth_hlen(dev);
>
> And cdc_ncm_set_dgram_size() updates ctx->max_datagram_size with the new
> mtu, meaning that you can only reduce the mtu. We should probably use
> cdc_ncm_max_dgram_size() instead here.
>
> And cdc_ncm_set_dgram_size() takes the datagram size with header as
> input (ref the above maxmtu calucalution), so it probably needs to
> called as
>
> cdc_ncm_set_dgram_size(dev, new_mtu + cdc_ncm_eth_hlen(dev));
>
> to get it right. I think. None of this is tested on an actual device
> yet... Care to test if I'm right, and do a v2 if necessry?
>
>> Cc: <stable@vger.kernel.org>
>
> This should be dropped for net. Ask David to queue it for stable
> instead. I usually do that by using a subject prefix like
>
> [PATCH net,stable v1] ...
>
>
>
>
> Bjørn
>
ok, thanks for feedback I will send v2 patch
next prev parent reply other threads:[~2016-04-11 11:13 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-04-06 11:41 [PATCH v1] net: cdc_ncm: update datagram size after changing mtu Robert Dobrowolski
2016-04-06 14:36 ` Bjørn Mork
2016-04-11 12:00 ` Robert Dobrowolski [this message]
2016-04-11 12:00 ` Robert Dobrowolski
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=52026.10.237.143.103.1460376036.squirrel@linux.intel.com \
--to=robert.dobrowolski@linux.intel.com \
--cc=bjorn@mork.no \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-usb@vger.kernel.org \
--cc=netdev@vger.kernel.org \
--cc=oliver@neukum.org \
--cc=rafal.f.redzimski@intel.com \
--cc=stable@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.