From: "Grumbach, Emmanuel" <emmanuel.grumbach@intel.com>
To: "xerofoify@gmail.com" <xerofoify@gmail.com>
Cc: "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"Spinadel, David" <david.spinadel@intel.com>,
"ilw@linux.intel.com" <ilw@linux.intel.com>,
"Coelho, Luciano" <luciano.coelho@intel.com>,
"eliad@wizery.com" <eliad@wizery.com>,
"Berg, Johannes" <johannes.berg@intel.com>,
"kvalo@codeaurora.org" <kvalo@codeaurora.org>,
"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
"linux-wireless@vger.kernel.org" <linux-wireless@vger.kernel.org>
Subject: Re: [PATCH] iwlwifi:dvm:Return false if resume command data is not same size as received packet for the function iwl_resume_status_fn
Date: Wed, 10 Jun 2015 17:10:09 +0000 [thread overview]
Message-ID: <1433956209.20602.5.camel@intel.com> (raw)
In-Reply-To: <D665C857-C846-422A-A3D9-A9314B000CD6@gmail.com>
T24gV2VkLCAyMDE1LTA2LTEwIGF0IDEyOjU4IC0wNDAwLCBOaWNob2xhcyBLcmF1c2Ugd3JvdGU6
DQo+IA0KPiBPbiBKdW5lIDEwLCAyMDE1IDEyOjUwOjQ1IFBNIEVEVCwgIkdydW1iYWNoLCBFbW1h
bnVlbCIgPGVtbWFudWVsLmdydW1iYWNoQGludGVsLmNvbT4gd3JvdGU6DQo+ID5PbiBXZWQsIDIw
MTUtMDYtMTAgYXQgMTI6MzMgLTA0MDAsIE5pY2hvbGFzIEtyYXVzZSB3cm90ZToNCj4gPj4gVGhp
cyBtYWtlcyB0aGUgZnVuY3Rpb24gaXdsX3Jlc3VtZV9zdGF0dXNfZm4gcmV0dXJuIGZhbHNlIG5v
dyBpZg0KPiA+PiB0aGUgcmVjZWl2ZWQgcGFja2V0IG9mIHR5cGUgaXdsX3J4X3BhY2tldCBpcyBu
b3QgdGhlIHNhbWUgc2l6ZQ0KPiA+PiBhcyB0aGUgc3RydWN0dXJlIHBvaW50ZXIsIGl3bF9yZXN1
bWVfZGF0YSdzIGNtZCBlbGVtZW50IGluIG9yZGVyDQo+ID4+IHRvIHNpZ25hbCBjYWxsZXJzIGFi
b3V0IHRoaXMgZXJyb3IgYW5kIGFsbG93IHRoZW0gdG8gaGFuZGxlIGl0DQo+ID4+IG9jY3VycmVu
dGx5Lg0KPiA+PiANCj4gPg0KPiA+SG0uLi4gRGlkIHlvdSBhY3R1YWxseSBoaXQgdGhpcyBpZj8N
Cj4gPkkgYW0gbm90IHN1cmUgSSByZWFsbHkgd2FudCB0byB3YWl0IGhlcmUgKHdoaWNoIGlzIHdo
YXQgd2lsbCBoYXBwZW4gaWYNCj4gPnlvdSByZXR1cm4gZmFsc2UpIHdoZW4gd2UgZ2V0IGFuIHVu
ZXhwZWN0ZWQgbGVuZ3RoPyBJIGRvIG5vdCBleHBlY3QNCj4gPmFueXRoaW5nIGJlc2lkZXMgdGhl
IHJlc3BvbnNlIEkgYW0gd2FpdGluZyBmb3Igc2luY2UgdGhlIGZpcm13YXJlIGlzDQo+ID5oYW5k
bGluZyB0aGUgR0VUX1NUQVRVUyAqb25seSogLSBpdCBqdXN0IGNhbWUgYmFjayBmcm9tIFdvV0xB
Ti4gQm90dG9tDQo+ID5saW5lLCB0aGlzIGlzIHJlYWxseSBhbiBlcnJvciBwYXRoIGFuZCBJIHBy
ZWZlciB0byBleGl0IGFuZCBub3Qgd2FpdA0KPiA+Zm9yDQo+ID50aGUgdGltZW91dCBpbiB0aGF0
IGNhc2UuDQo+ID5CdXQgSSBtaWdodCBiZSBtaXNzaW5nIHNvbWV0aGluZz8NCj4gPg0KPiBXaHkg
bm90IHdhaXQgZm9yIHRoZSB0aW1lIG91dD8gIFNlZW1zIHRoZXJlIGlzIG5vIHJlYXNvbiBub3Qg
dG8gYW5kIGluIHRoYXQgY2FzZQ0KPiBpZiB0aGUgZmlybXdhcmUgaGFuZGxlcyB0aGlzDQoNCkkg
ZG91YnQgaXQgd2lsbC4gVGhpcyBnb2VzIGJhY2sgdG8gbXkgb3JpZ2luYWwgcXVlc3Rpb246IGRp
ZCB5b3UgcmVhbGx5DQpoaXQgdGhpcyBwYXRoPyBEb2VzIHRoZSBwYXRjaCBzb2x2ZXMgYSByZWFs
IGJ1ZyB5b3UgZmFjZWQ/DQoNCj4gd2h5IGlzIHRoZSBpZiBzdGF0ZW1lbnQgc3RpbGwgaGVyZS4g
DQoNClRvIGdldCBhIGRlYnVnIHByaW50PyA6KQ0KDQo+IE5pY2sgDQo+ID4+IFNpZ25lZC1vZmYt
Ynk6IE5pY2hvbGFzIEtyYXVzZSA8eGVyb2ZvaWZ5QGdtYWlsLmNvbT4NCj4gPj4gLS0tDQo+ID4+
ICBkcml2ZXJzL25ldC93aXJlbGVzcy9pd2x3aWZpL2R2bS9tYWM4MDIxMS5jIHwgMiArLQ0KPiA+
PiAgMSBmaWxlIGNoYW5nZWQsIDEgaW5zZXJ0aW9uKCspLCAxIGRlbGV0aW9uKC0pDQo+ID4+IA0K
PiA+PiBkaWZmIC0tZ2l0IGEvZHJpdmVycy9uZXQvd2lyZWxlc3MvaXdsd2lmaS9kdm0vbWFjODAy
MTEuYw0KPiA+Yi9kcml2ZXJzL25ldC93aXJlbGVzcy9pd2x3aWZpL2R2bS9tYWM4MDIxMS5jDQo+
ID4+IGluZGV4IDVhYmQ2MmUuLjIxZTgwOGMgMTAwNjQ0DQo+ID4+IC0tLSBhL2RyaXZlcnMvbmV0
L3dpcmVsZXNzL2l3bHdpZmkvZHZtL21hYzgwMjExLmMNCj4gPj4gKysrIGIvZHJpdmVycy9uZXQv
d2lyZWxlc3MvaXdsd2lmaS9kdm0vbWFjODAyMTEuYw0KPiA+PiBAQCAtNDA5LDcgKzQwOSw3IEBA
IHN0YXRpYyBib29sIGl3bF9yZXN1bWVfc3RhdHVzX2ZuKHN0cnVjdA0KPiA+aXdsX25vdGlmX3dh
aXRfZGF0YSAqbm90aWZfd2FpdCwNCj4gPj4gIA0KPiA+PiAgCWlmIChpd2xfcnhfcGFja2V0X3Bh
eWxvYWRfbGVuKHBrdCkgIT0gc2l6ZW9mKCpyZXN1bWVfZGF0YS0+Y21kKSkgew0KPiA+PiAgCQlJ
V0xfRVJSKHByaXYsICJyeCB3cm9uZyBzaXplIGRhdGFcbiIpOw0KPiA+PiAtCQlyZXR1cm4gdHJ1
ZTsNCj4gPj4gKwkJcmV0dXJuIGZhbHNlOw0KPiA+PiAgCX0NCj4gPj4gIAltZW1jcHkocmVzdW1l
X2RhdGEtPmNtZCwgcGt0LT5kYXRhLCBzaXplb2YoKnJlc3VtZV9kYXRhLT5jbWQpKTsNCj4gPj4g
IAlyZXN1bWVfZGF0YS0+dmFsaWQgPSB0cnVlOw0KPiANCg0K
WARNING: multiple messages have this Message-ID (diff)
From: "Grumbach, Emmanuel" <emmanuel.grumbach@intel.com>
To: "xerofoify@gmail.com" <xerofoify@gmail.com>
Cc: "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"Spinadel, David" <david.spinadel@intel.com>,
"ilw@linux.intel.com" <ilw@linux.intel.com>,
"Coelho, Luciano" <luciano.coelho@intel.com>,
"eliad@wizery.com" <eliad@wizery.com>,
"Berg, Johannes" <johannes.berg@intel.com>,
"kvalo@codeaurora.org" <kvalo@codeaurora.org>,
"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
"linux-wireless@vger.kernel.org" <linux-wireless@vger.kernel.org>
Subject: Re: [PATCH] iwlwifi:dvm:Return false if resume command data is not same size as received packet for the function iwl_resume_status_fn
Date: Wed, 10 Jun 2015 17:10:09 +0000 [thread overview]
Message-ID: <1433956209.20602.5.camel@intel.com> (raw)
In-Reply-To: <D665C857-C846-422A-A3D9-A9314B000CD6@gmail.com>
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2308 bytes --]
On Wed, 2015-06-10 at 12:58 -0400, Nicholas Krause wrote:
>
> On June 10, 2015 12:50:45 PM EDT, "Grumbach, Emmanuel" <emmanuel.grumbach@intel.com> wrote:
> >On Wed, 2015-06-10 at 12:33 -0400, Nicholas Krause wrote:
> >> This makes the function iwl_resume_status_fn return false now if
> >> the received packet of type iwl_rx_packet is not the same size
> >> as the structure pointer, iwl_resume_data's cmd element in order
> >> to signal callers about this error and allow them to handle it
> >> occurrently.
> >>
> >
> >Hm... Did you actually hit this if?
> >I am not sure I really want to wait here (which is what will happen if
> >you return false) when we get an unexpected length? I do not expect
> >anything besides the response I am waiting for since the firmware is
> >handling the GET_STATUS *only* - it just came back from WoWLAN. Bottom
> >line, this is really an error path and I prefer to exit and not wait
> >for
> >the timeout in that case.
> >But I might be missing something?
> >
> Why not wait for the time out? Seems there is no reason not to and in that case
> if the firmware handles this
I doubt it will. This goes back to my original question: did you really
hit this path? Does the patch solves a real bug you faced?
> why is the if statement still here.
To get a debug print? :)
> Nick
> >> Signed-off-by: Nicholas Krause <xerofoify@gmail.com>
> >> ---
> >> drivers/net/wireless/iwlwifi/dvm/mac80211.c | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/net/wireless/iwlwifi/dvm/mac80211.c
> >b/drivers/net/wireless/iwlwifi/dvm/mac80211.c
> >> index 5abd62e..21e808c 100644
> >> --- a/drivers/net/wireless/iwlwifi/dvm/mac80211.c
> >> +++ b/drivers/net/wireless/iwlwifi/dvm/mac80211.c
> >> @@ -409,7 +409,7 @@ static bool iwl_resume_status_fn(struct
> >iwl_notif_wait_data *notif_wait,
> >>
> >> if (iwl_rx_packet_payload_len(pkt) != sizeof(*resume_data->cmd)) {
> >> IWL_ERR(priv, "rx wrong size data\n");
> >> - return true;
> >> + return false;
> >> }
> >> memcpy(resume_data->cmd, pkt->data, sizeof(*resume_data->cmd));
> >> resume_data->valid = true;
>
ÿôèº{.nÇ+‰·Ÿ®‰†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a¢f£¢·hšïêÿ‘êçz_è®\x03(階ŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨èÚ&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥
WARNING: multiple messages have this Message-ID (diff)
From: "Grumbach, Emmanuel" <emmanuel.grumbach@intel.com>
To: "xerofoify@gmail.com" <xerofoify@gmail.com>
Cc: "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"Spinadel, David" <david.spinadel@intel.com>,
"ilw@linux.intel.com" <ilw@linux.intel.com>,
"Coelho, Luciano" <luciano.coelho@intel.com>,
"eliad@wizery.com" <eliad@wizery.com>,
"Berg, Johannes" <johannes.berg@intel.com>,
"kvalo@codeaurora.org" <kvalo@codeaurora.org>,
"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
"linux-wireless@vger.kernel.org" <linux-wireless@vger.kernel.org>
Subject: Re: [PATCH] iwlwifi:dvm:Return false if resume command data is not same size as received packet for the function iwl_resume_status_fn
Date: Wed, 10 Jun 2015 17:10:09 +0000 [thread overview]
Message-ID: <1433956209.20602.5.camel@intel.com> (raw)
In-Reply-To: <D665C857-C846-422A-A3D9-A9314B000CD6@gmail.com>
On Wed, 2015-06-10 at 12:58 -0400, Nicholas Krause wrote:
>
> On June 10, 2015 12:50:45 PM EDT, "Grumbach, Emmanuel" <emmanuel.grumbach@intel.com> wrote:
> >On Wed, 2015-06-10 at 12:33 -0400, Nicholas Krause wrote:
> >> This makes the function iwl_resume_status_fn return false now if
> >> the received packet of type iwl_rx_packet is not the same size
> >> as the structure pointer, iwl_resume_data's cmd element in order
> >> to signal callers about this error and allow them to handle it
> >> occurrently.
> >>
> >
> >Hm... Did you actually hit this if?
> >I am not sure I really want to wait here (which is what will happen if
> >you return false) when we get an unexpected length? I do not expect
> >anything besides the response I am waiting for since the firmware is
> >handling the GET_STATUS *only* - it just came back from WoWLAN. Bottom
> >line, this is really an error path and I prefer to exit and not wait
> >for
> >the timeout in that case.
> >But I might be missing something?
> >
> Why not wait for the time out? Seems there is no reason not to and in that case
> if the firmware handles this
I doubt it will. This goes back to my original question: did you really
hit this path? Does the patch solves a real bug you faced?
> why is the if statement still here.
To get a debug print? :)
> Nick
> >> Signed-off-by: Nicholas Krause <xerofoify@gmail.com>
> >> ---
> >> drivers/net/wireless/iwlwifi/dvm/mac80211.c | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/net/wireless/iwlwifi/dvm/mac80211.c
> >b/drivers/net/wireless/iwlwifi/dvm/mac80211.c
> >> index 5abd62e..21e808c 100644
> >> --- a/drivers/net/wireless/iwlwifi/dvm/mac80211.c
> >> +++ b/drivers/net/wireless/iwlwifi/dvm/mac80211.c
> >> @@ -409,7 +409,7 @@ static bool iwl_resume_status_fn(struct
> >iwl_notif_wait_data *notif_wait,
> >>
> >> if (iwl_rx_packet_payload_len(pkt) != sizeof(*resume_data->cmd)) {
> >> IWL_ERR(priv, "rx wrong size data\n");
> >> - return true;
> >> + return false;
> >> }
> >> memcpy(resume_data->cmd, pkt->data, sizeof(*resume_data->cmd));
> >> resume_data->valid = true;
>
next prev parent reply other threads:[~2015-06-10 17:10 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <1433954031-7176-1-git-send-email-xerofoify@gmail.com>
2015-06-10 16:50 ` [PATCH] iwlwifi:dvm:Return false if resume command data is not same size as received packet for the function iwl_resume_status_fn Grumbach, Emmanuel
2015-06-10 16:50 ` Grumbach, Emmanuel
2015-06-10 16:50 ` Grumbach, Emmanuel
[not found] ` <D665C857-C846-422A-A3D9-A9314B000CD6@gmail.com>
2015-06-10 17:10 ` Grumbach, Emmanuel [this message]
2015-06-10 17:10 ` Grumbach, Emmanuel
2015-06-10 17:10 ` Grumbach, Emmanuel
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=1433956209.20602.5.camel@intel.com \
--to=emmanuel.grumbach@intel.com \
--cc=david.spinadel@intel.com \
--cc=eliad@wizery.com \
--cc=ilw@linux.intel.com \
--cc=johannes.berg@intel.com \
--cc=kvalo@codeaurora.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-wireless@vger.kernel.org \
--cc=luciano.coelho@intel.com \
--cc=netdev@vger.kernel.org \
--cc=xerofoify@gmail.com \
/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.