Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1168666 > unrolled thread
| Started by | Maninder Singh <maninder1.s@samsung.com> |
|---|---|
| First post | 2015-06-19 12:00 +0200 |
| Last post | 2015-06-19 12:10 +0200 |
| Articles | 2 — 2 participants |
Back to article view | Back to linux.kernel
Re: [PATCH 1/1] irq-gic: use BUG_ON instead of if()/BUG Maninder Singh <maninder1.s@samsung.com> - 2015-06-19 12:00 +0200
Re: [PATCH 1/1] irq-gic: use BUG_ON instead of if()/BUG Thomas Gleixner <tglx@linutronix.de> - 2015-06-19 12:10 +0200
| From | Maninder Singh <maninder1.s@samsung.com> |
|---|---|
| Date | 2015-06-19 12:00 +0200 |
| Subject | Re: [PATCH 1/1] irq-gic: use BUG_ON instead of if()/BUG |
| Message-ID | <pD1fB-3NZ-31@gated-at.bofh.it> |
SGkgVGhvbWFzLA0KDQo+PiAgew0KPj4gLQlpZiAoZ2ljX25yID49IE1BWF9HSUNfTlIpDQo+PiAt CQlCVUcoKTsNCj4+ICsJQlVHX09OKGdpY19uciA+PSBNQVhfR0lDX05SKTsNCj4+ICAJaWYgKGly cV9zZXRfaGFuZGxlcl9kYXRhKGlycSwgJmdpY19kYXRhW2dpY19ucl0pICE9IDApDQo+PiAgCQlC VUcoKTsNCj4NCj5TbyB0aGlzIHBhdGNoIHdhcyBjbGVhcmx5IGRvbmUganVzdCBieSBydW5uaW5n IGEgc2NyaXB0IGFuZCBub3Qgc2FuaXR5DQo+Y2hlY2tlZCBhZnRlcndhcmRzLiBPdGhlcndpc2Ug dGhlIG5leHQgaWYoKSBCVUcoKTsgY29uc3RydWN0IHdvdWxkDQo+aGF2ZSBiZWVuIGZpeGVkIGFz IHdlbGwuDQoNClllcyBzZW1hbnRpYyBwYXRjaCBkaWQgdGhlIGNoYW5nZXMgdG8gdXNlIHByZWZl cnJlZCBBUElzLA0KQW5kIGl0IGFsc28gY2hhbmdlZCB0aGlzIEJVR19PTihpcnFfc2V0X2hhbmRs ZXJfZGF0YShpcnEsICZnaWNfZGF0YVtnaWNfbnJdKSAhPSAwKQ0KQnV0IHdlIGhhdmUgdG8gdGFr ZSBjYXJlIHRoYXQgIGNvbmRpdGlvbiBoYXMgbm8gc2lkZSBlZmZlY3RzIGkuZS4NCg0KaWYoKS9C VUcgY29udmVyc2lvbiB0byBCVUdfT04gbXVzdCBiZSBhdm9pZGVkIHdoZW4gdGhlcmUncyBzaWRl IGVmZmVjdA0KaW4gY29uZGl0aW9uLiBUaGUgcmVhc29uIGJlaW5nIEJVR19PTiB3b24ndCBleGVj dXRlIHRoZSBjb25kaXRpb24gd2hlbiBDT05GSUdfQlVHDQppcyBub3QgZGVmaW5lZCBBcyBzdWdn ZXN0ZWQgYnkgSnVsaWEgTGF3YWxsDQoNClRoYXRzIHdoeSBkaWQgbm90IHRha2UgdGhhdCBjaGFu Z2UgIC0tPiAoQlVHX09OKGlycV9zZXRfaGFuZGxlcl9kYXRhKGlycSwgJmdpY19kYXRhW2dpY19u cl0pICE9IDApKQ0KDQo+RnVydGhlciwgd2hpbGUgd2UgYXJlIGF0IHRoYXQuIEl0IHdvdWxkIGJl IGV2ZW4gbW9yZSB1c2VmdWwgdG8gYW5hbHl6ZQ0KPndoZXRoZXIgdGhlIEJVR19PTigpIGlzIG5l ZWRlZCBpbiB0aGUgZmlyc3QgcGxhY2Ugb3IgYXQgbGVhc3QgY291bGQgYmUNCj5tYWRlIGNvbmRp dGlvbmFsIG9uIHNvbWUgZGVidWcgb3B0aW9uLg0KPg0KPkJ1dCB0aGF0J3Mgbm90IGRvbmUgYnkg dGhlIHNjcmlwdCBlaXRoZXIsIHJpZ2h0Pw0KDQpZZXMgY29jY2luZWxsZSBzZW1hbnRpYyBwYXRj aGVzIGRpZCBub3QgZG8gdGhhdCBjaGFuZ2VzLg0Kd2UgaGF2ZSB0byBjaG9vc2Ugd2hldGhlciB0 byBtYWtlIEJVR19PTiBjb25kaXRpb25hbCBvbiBzb21lIGRlYnVnIG9wdGlvbnMuDQoNClRoYW5r cywNCk1hbmluZGVyDQoNCg0K -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [next] | [standalone]
| From | Thomas Gleixner <tglx@linutronix.de> |
|---|---|
| Date | 2015-06-19 12:10 +0200 |
| Message-ID | <pD1pf-4ew-11@gated-at.bofh.it> |
| In reply to | #1168666 |
On Fri, 19 Jun 2015, Maninder Singh wrote:
> Hi Thomas,
>
> >> {
> >> - if (gic_nr >= MAX_GIC_NR)
> >> - BUG();
> >> + BUG_ON(gic_nr >= MAX_GIC_NR);
> >> if (irq_set_handler_data(irq, &gic_data[gic_nr]) != 0)
> >> BUG();
> >
> >So this patch was clearly done just by running a script and not sanity
> >checked afterwards. Otherwise the next if() BUG(); construct would
> >have been fixed as well.
>
> Yes semantic patch did the changes to use preferred APIs,
> And it also changed this BUG_ON(irq_set_handler_data(irq, &gic_data[gic_nr]) != 0)
> But we have to take care that condition has no side effects i.e.
>
> if()/BUG conversion to BUG_ON must be avoided when there's side effect
> in condition. The reason being BUG_ON won't execute the condition when CONFIG_BUG
> is not defined As suggested by Julia Lawall
>
> Thats why did not take that change --> (BUG_ON(irq_set_handler_data(irq, &gic_data[gic_nr]) != 0))
Fair enough. But you should mention that in the changelog.
> >Further, while we are at that. It would be even more useful to analyze
> >whether the BUG_ON() is needed in the first place or at least could be
> >made conditional on some debug option.
> >
> >But that's not done by the script either, right?
>
> Yes coccinelle semantic patches did not do that changes.
> we have to choose whether to make BUG_ON conditional on some debug options.
Right, and that's what I'm asking for. IOW, instead of blindly running
scripts at least ask the question whether this stuff needs to be there
unconditionally....
Such information can be put into the changelog and helps the reviewers
to distinguish thoughtful people from script bots.
Thanks,
tglx
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web