From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752883AbdLHRmI (ORCPT ); Fri, 8 Dec 2017 12:42:08 -0500 Received: from iolanthe.rowland.org ([192.131.102.54]:46870 "HELO iolanthe.rowland.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S1752003AbdLHRmG (ORCPT ); Fri, 8 Dec 2017 12:42:06 -0500 Date: Fri, 8 Dec 2017 12:42:05 -0500 (EST) From: Alan Stern X-X-Sender: stern@iolanthe.rowland.org To: Geert Uytterhoeven 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" Subject: Re: USB: hub: Delete an error message for a failed memory allocation in usb_hub_clear_tt_buffer() In-Reply-To: Message-ID: MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, 8 Dec 2017, Geert Uytterhoeven wrote: > 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? I can't see how to make that work. We don't know beforehand how many structures will be needed at any time. Alan Stern From mboxrd@z Thu Jan 1 00:00:00 1970 From: Alan Stern Date: Fri, 08 Dec 2017 17:42:05 +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: Geert Uytterhoeven 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" On Fri, 8 Dec 2017, Geert Uytterhoeven wrote: > 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? I can't see how to make that work. We don't know beforehand how many structures will be needed at any time. Alan Stern 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: Alan Stern Message-Id: Date: Fri, 8 Dec 2017 12:42:05 -0500 (EST) To: Geert Uytterhoeven 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: T24gRnJpLCA4IERlYyAyMDE3LCBHZWVydCBVeXR0ZXJob2V2ZW4gd3JvdGU6Cgo+IEhpIEFsYW4s Cj4gCj4gT24gVGh1LCBEZWMgNywgMjAxNyBhdCAxMDoyNiBQTSwgQWxhbiBTdGVybiA8c3Rlcm5A cm93bGFuZC5oYXJ2YXJkLmVkdT4gd3JvdGU6Cj4gPiBPbiBUaHUsIDcgRGVjIDIwMTcsIERhbiBD YXJwZW50ZXIgd3JvdGU6Cj4gPj4gVGhlIHN0YW5kYXJkIGlzIHRvIHRyZWF0IHRoZW0gbGlrZSBl cnJvcnMgYW5kIHRyeSBwcmVzcyBmb3J3YXJkIGluIGEKPiA+PiBkZWdyYWRlZCBtb2RlIGJ1dCBk b24ndCBwcmludCBhIG1lc3NhZ2UuICBDaGVja3BhdGNoLnBsIGNvbXBsYWlucyBpZiB5b3UKPiA+ PiBwcmludCBhIHdhcm5pbmcgZm9yIGFsbG9jYXRpb24gZmFpbHVyZXMuICBBIGxvdCBvZiBsb3cg bGV2ZWwgZnVuY3Rpb25zCj4gPj4gaGFuZGxlIHRoZWlyIG93biBtZXNzYWdlcyBwcmV0dHkgd2Vs bCBidXQgZXNwZWNpYWxseSBrbWFsbG9jKCkgZG9lcy4KPiA+Cj4gPiBXaGljaCBicmluZ3MgdXMg YmFjayB0byBteSBvcmlnaW5hbCBvYmplY3Rpb24uICBJZiBhbiBhbGxvY2F0aW9uCj4gPiBmYWls dXJlIGhhcyBsb2NhbGl6ZWQgZWZmZWN0cywgYnV0IHRoZSBsb3ctbGV2ZWwgd2FybmluZyBpcyB1 bmFibGUgdG8KPiA+IHNwZWNpZnkgd2hhdCB3aWxsIGJlIGFmZmVjdGVkLCBob3cgaXMgdGhlIHVz ZXIgc3VwcG9zZWQgdG8gY29ubmVjdCB0aGUKPiA+IGVmZmVjdCB0byB0aGUgY2F1c2U/Cj4gCj4g VGhlIGJhY2t0cmFjZSB3b3VsZCBpbmNsdWRlIHVzYl9odWJfY2xlYXJfdHRfYnVmZmVyLCBzbyB0 aGUgdXNlciB3aWxsCj4ga25vdyBVU0IgaXMgYWZmZWN0ZWQuCj4gTm90ZSB0aGF0IHRoZSBjYXVz ZSBvZiB0aGUgbWVtb3J5IGV4aGF1c3Rpb24gaXMgdXN1YWxseSBub3QgdGhlIGNhbGxlci4KPiBV bmxlc3MgaXQgcmVxdWVzdHMgYSByZWFsIGJpZyBibG9jayBvZiBtZW1vcnksIGluIHdoaWNoIGNh c2UgdGhhdCB3aWxsIGJlCj4gbWVudGlvbmVkIGluIHRoZSBiYWNrdHJhY2UsIHRvby4KPiAKPiBJ biB0aGlzIHBhcnRpY3VsYXIgY2FzZSwgdGhlIGRyaXZlciB1c2VzIEdGUF9BVE9NSUMsIHNvIGFs bG9jYXRpb24gZmFpbHVyZXMKPiBhcmVuJ3QgdGhhdCByYXJlLCBhbmQgSSB0aGluayB0aGUgZHJp dmVyIHNob3VsZCBiZSBwcmVwYXJlZCBmb3IgdGhhdCwgYW5kIHRyeQo+IHRvIHJlY292ZXIgZ3Jh Y2VmdWxseS4KPiAKPiBUaGUgY29tbWVudAo+IAo+ICAgICAgICAgICAgICAgICAvKiBGSVhNRSBy ZWNvdmVyIHNvbWVob3cgLi4uIFJFU0VUX1RUPyAqLwo+IAo+IG1ha2VzIG1lIHRoaW5rIGl0IGlz bid0Lgo+IAo+IEFzIHRoaXMgaXMgYSBwcmV0dHkgc21hbGwgYWxsb2NhdGlvbiwgcGVyaGFwcyBp dCBjYW4gYmUgZG9uZSBiZWZvcmVoYW5kLCB3aXRob3V0Cj4gR0ZQX0FUT01JQz8KCkkgY2FuJ3Qg c2VlIGhvdyB0byBtYWtlIHRoYXQgd29yay4gIFdlIGRvbid0IGtub3cgYmVmb3JlaGFuZCBob3cg bWFueQpzdHJ1Y3R1cmVzIHdpbGwgYmUgbmVlZGVkIGF0IGFueSB0aW1lLgoKQWxhbiBTdGVybgot LS0KVG8gdW5zdWJzY3JpYmUgZnJvbSB0aGlzIGxpc3Q6IHNlbmQgdGhlIGxpbmUgInVuc3Vic2Ny aWJlIGxpbnV4LXVzYiIgaW4KdGhlIGJvZHkgb2YgYSBtZXNzYWdlIHRvIG1ham9yZG9tb0B2Z2Vy Lmtlcm5lbC5vcmcKTW9yZSBtYWpvcmRvbW8gaW5mbyBhdCAgaHR0cDovL3ZnZXIua2VybmVsLm9y Zy9tYWpvcmRvbW8taW5mby5odG1sCg==