From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752691AbdLHHnk (ORCPT ); Fri, 8 Dec 2017 02:43:40 -0500 Received: from mail-qt0-f196.google.com ([209.85.216.196]:32869 "EHLO mail-qt0-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752010AbdLHHnj (ORCPT ); Fri, 8 Dec 2017 02:43:39 -0500 X-Google-Smtp-Source: AGs4zMZDezOuavDs7I/ThdFiokFW//CUNHoTJU9R6xrK0ZT5oh0L6pe8vazj2q/goGZBLjDdxFhS+Y6b7yJn2jio7C4= MIME-Version: 1.0 In-Reply-To: References: <20171207204953.5gsk26pk4b5xaizq@mwanda> From: Geert Uytterhoeven Date: Fri, 8 Dec 2017 08:43:37 +0100 X-Google-Sender-Auth: 0GK4bTigwp9wJD4rq8Xspa71TZA Message-ID: Subject: Re: USB: hub: Delete an error message for a failed memory allocation in usb_hub_clear_tt_buffer() To: Alan Stern Cc: Dan Carpenter , SF Markus Elfring , USB list , Joe Perches , Daniel Drake , Dmitry Fleytman , Eugene Korenevsky , Greg Kroah-Hartman , =?UTF-8?B?R8O8bnRlciBSw7Zjaw==?= , Johan Hovold , Mathias Nyman , Peter Chen , LKML , "kernel-janitors@vger.kernel.org" Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Alan, On Thu, Dec 7, 2017 at 10:26 PM, Alan Stern wrote: > On Thu, 7 Dec 2017, Dan Carpenter wrote: >> The standard is to treat them like errors and try press forward in a >> degraded mode but don't print a message. Checkpatch.pl complains if you >> print a warning for allocation failures. A lot of low level functions >> handle their own messages pretty well but especially kmalloc() does. > > Which brings us back to my original objection. If an allocation > failure has localized effects, but the low-level warning is unable to > specify what will be affected, how is the user supposed to connect the > effect to the cause? The backtrace would include usb_hub_clear_tt_buffer, so the user will know USB is affected. Note that the cause of the memory exhaustion is usually not the caller. Unless it requests a real big block of memory, in which case that will be mentioned in the backtrace, too. In this particular case, the driver uses GFP_ATOMIC, so allocation failures aren't that rare, and I think the driver should be prepared for that, and try to recover gracefully. The comment /* FIXME recover somehow ... RESET_TT? */ makes me think it isn't. As this is a pretty small allocation, perhaps it can be done beforehand, without GFP_ATOMIC? Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds From mboxrd@z Thu Jan 1 00:00:00 1970 From: Geert Uytterhoeven Date: Fri, 08 Dec 2017 07:43:37 +0000 Subject: Re: USB: hub: Delete an error message for a failed memory allocation in usb_hub_clear_tt_buffer() Message-Id: List-Id: References: <97b0eeb8-834e-61ca-01dd-afbbf18697db@users.sourceforge.net> In-Reply-To: <97b0eeb8-834e-61ca-01dd-afbbf18697db@users.sourceforge.net> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Alan Stern Cc: Dan Carpenter , SF Markus Elfring , USB list , Joe Perches , Daniel Drake , Dmitry Fleytman , Eugene Korenevsky , Greg Kroah-Hartman , =?UTF-8?B?R8O8bnRlciBSw7Zjaw==?= , Johan Hovold , Mathias Nyman , Peter Chen , LKML , "kernel-janitors@vger.kernel.org" Hi Alan, On Thu, Dec 7, 2017 at 10:26 PM, Alan Stern wrote: > On Thu, 7 Dec 2017, Dan Carpenter wrote: >> The standard is to treat them like errors and try press forward in a >> degraded mode but don't print a message. Checkpatch.pl complains if you >> print a warning for allocation failures. A lot of low level functions >> handle their own messages pretty well but especially kmalloc() does. > > Which brings us back to my original objection. If an allocation > failure has localized effects, but the low-level warning is unable to > specify what will be affected, how is the user supposed to connect the > effect to the cause? The backtrace would include usb_hub_clear_tt_buffer, so the user will know USB is affected. Note that the cause of the memory exhaustion is usually not the caller. Unless it requests a real big block of memory, in which case that will be mentioned in the backtrace, too. In this particular case, the driver uses GFP_ATOMIC, so allocation failures aren't that rare, and I think the driver should be prepared for that, and try to recover gracefully. The comment /* FIXME recover somehow ... RESET_TT? */ makes me think it isn't. As this is a pretty small allocation, perhaps it can be done beforehand, without GFP_ATOMIC? Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds From mboxrd@z Thu Jan 1 00:00:00 1970 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Subject: USB: hub: Delete an error message for a failed memory allocation in usb_hub_clear_tt_buffer() From: Geert Uytterhoeven Message-Id: Date: Fri, 8 Dec 2017 08:43:37 +0100 To: Alan Stern Cc: Dan Carpenter , SF Markus Elfring , USB list , Joe Perches , Daniel Drake , Dmitry Fleytman , Eugene Korenevsky , Greg Kroah-Hartman , =?UTF-8?B?R8O8bnRlciBSw7Zjaw==?= , Johan Hovold , Mathias Nyman , Peter Chen , LKML , "kernel-janitors@vger.kernel.org" List-ID: SGkgQWxhbiwKCk9uIFRodSwgRGVjIDcsIDIwMTcgYXQgMTA6MjYgUE0sIEFsYW4gU3Rlcm4gPHN0 ZXJuQHJvd2xhbmQuaGFydmFyZC5lZHU+IHdyb3RlOgo+IE9uIFRodSwgNyBEZWMgMjAxNywgRGFu IENhcnBlbnRlciB3cm90ZToKPj4gVGhlIHN0YW5kYXJkIGlzIHRvIHRyZWF0IHRoZW0gbGlrZSBl cnJvcnMgYW5kIHRyeSBwcmVzcyBmb3J3YXJkIGluIGEKPj4gZGVncmFkZWQgbW9kZSBidXQgZG9u J3QgcHJpbnQgYSBtZXNzYWdlLiAgQ2hlY2twYXRjaC5wbCBjb21wbGFpbnMgaWYgeW91Cj4+IHBy aW50IGEgd2FybmluZyBmb3IgYWxsb2NhdGlvbiBmYWlsdXJlcy4gIEEgbG90IG9mIGxvdyBsZXZl bCBmdW5jdGlvbnMKPj4gaGFuZGxlIHRoZWlyIG93biBtZXNzYWdlcyBwcmV0dHkgd2VsbCBidXQg ZXNwZWNpYWxseSBrbWFsbG9jKCkgZG9lcy4KPgo+IFdoaWNoIGJyaW5ncyB1cyBiYWNrIHRvIG15 IG9yaWdpbmFsIG9iamVjdGlvbi4gIElmIGFuIGFsbG9jYXRpb24KPiBmYWlsdXJlIGhhcyBsb2Nh bGl6ZWQgZWZmZWN0cywgYnV0IHRoZSBsb3ctbGV2ZWwgd2FybmluZyBpcyB1bmFibGUgdG8KPiBz cGVjaWZ5IHdoYXQgd2lsbCBiZSBhZmZlY3RlZCwgaG93IGlzIHRoZSB1c2VyIHN1cHBvc2VkIHRv IGNvbm5lY3QgdGhlCj4gZWZmZWN0IHRvIHRoZSBjYXVzZT8KClRoZSBiYWNrdHJhY2Ugd291bGQg aW5jbHVkZSB1c2JfaHViX2NsZWFyX3R0X2J1ZmZlciwgc28gdGhlIHVzZXIgd2lsbAprbm93IFVT QiBpcyBhZmZlY3RlZC4KTm90ZSB0aGF0IHRoZSBjYXVzZSBvZiB0aGUgbWVtb3J5IGV4aGF1c3Rp b24gaXMgdXN1YWxseSBub3QgdGhlIGNhbGxlci4KVW5sZXNzIGl0IHJlcXVlc3RzIGEgcmVhbCBi aWcgYmxvY2sgb2YgbWVtb3J5LCBpbiB3aGljaCBjYXNlIHRoYXQgd2lsbCBiZQptZW50aW9uZWQg aW4gdGhlIGJhY2t0cmFjZSwgdG9vLgoKSW4gdGhpcyBwYXJ0aWN1bGFyIGNhc2UsIHRoZSBkcml2 ZXIgdXNlcyBHRlBfQVRPTUlDLCBzbyBhbGxvY2F0aW9uIGZhaWx1cmVzCmFyZW4ndCB0aGF0IHJh cmUsIGFuZCBJIHRoaW5rIHRoZSBkcml2ZXIgc2hvdWxkIGJlIHByZXBhcmVkIGZvciB0aGF0LCBh bmQgdHJ5CnRvIHJlY292ZXIgZ3JhY2VmdWxseS4KClRoZSBjb21tZW50CgogICAgICAgICAgICAg ICAgLyogRklYTUUgcmVjb3ZlciBzb21laG93IC4uLiBSRVNFVF9UVD8gKi8KCm1ha2VzIG1lIHRo aW5rIGl0IGlzbid0LgoKQXMgdGhpcyBpcyBhIHByZXR0eSBzbWFsbCBhbGxvY2F0aW9uLCBwZXJo YXBzIGl0IGNhbiBiZSBkb25lIGJlZm9yZWhhbmQsIHdpdGhvdXQKR0ZQX0FUT01JQz8KCkdye29l dGplLGVldGluZ31zLAoKICAgICAgICAgICAgICAgICAgICAgICAgR2VlcnQKLS0tCkdlZXJ0IFV5 dHRlcmhvZXZlbiAtLSBUaGVyZSdzIGxvdHMgb2YgTGludXggYmV5b25kIGlhMzIgLS0gZ2VlcnRA bGludXgtbTY4ay5vcmcKCkluIHBlcnNvbmFsIGNvbnZlcnNhdGlvbnMgd2l0aCB0ZWNobmljYWwg cGVvcGxlLCBJIGNhbGwgbXlzZWxmIGEgaGFja2VyLiBCdXQKd2hlbiBJJ20gdGFsa2luZyB0byBq b3VybmFsaXN0cyBJIGp1c3Qgc2F5ICJwcm9ncmFtbWVyIiBvciBzb21ldGhpbmcgbGlrZSB0aGF0 LgogICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgIC0tIExpbnVzIFRvcnZhbGRzCi0tClRv IHVuc3Vic2NyaWJlIGZyb20gdGhpcyBsaXN0OiBzZW5kIHRoZSBsaW5lICJ1bnN1YnNjcmliZSBs aW51eC11c2IiIGluCnRoZSBib2R5IG9mIGEgbWVzc2FnZSB0byBtYWpvcmRvbW9Admdlci5rZXJu ZWwub3JnCk1vcmUgbWFqb3Jkb21vIGluZm8gYXQgIGh0dHA6Ly92Z2VyLmtlcm5lbC5vcmcvbWFq b3Jkb21vLWluZm8uaHRtbAo=