On Mon, 2015-02-16 at 17:56 +0700, Arseny Solokha wrote:
quoted
Drop unused fsl_mpic_primary_get_version(), mpic_set_clk_ratio(),
mpic_set_serial_int().
I'm always happy to remove unused code, but the interesting question is w=
hy are
they unused? Please tell me in the changelog.
To being able to give a definitive answer, it's necessary to understand
the intentions of original developers of these pieces. I just can tell
these functions have no users and trivial grepping easily proves it;
I've got the impression they are here only for the sake of
implementation completeness.
Two machines at hands, e300 and e500 based, boot and run without
regressions on my workload with this series applied. The removed code
seems also been rarely touched, so it seems the series is safe at least
in general. But I can't obviously express any strong point in support of
the series, so it's completely OK to leave things as is.
+ fsl_mpic_primary_get_version() is just a safe wrapper around
fsl_mpic_get_version() for SMP configurations. While the latter is
called explicitly for handling PIC initialization and setting up error
interrupt vector depending on PIC hardware version, the former isn't
used for anything.
+ As for mpic_set_clk_ratio() and mpic_set_serial_int(), they both
are almost nine years old[1] but still have no chance to be called even
from out-of-tree modules because they both are __init and of course
aren't exported. Non-demanded functionality?
Of course I'll include the last two paragraphs into the V2 patch
description if the explanation is convincing enough and you ACK it. If
the patch is safe it's also necessary to extend it a bit, making its
second part actually a complete revert of [1].
[1] https://lists.ozlabs.org/pipermail/linuxppc-dev/2006-June/023867.html
Ars=C3=A9ny
cheers
On Thu, 2015-02-19 at 19:26 +0700, Arseny Solokha wrote:
quoted
On Mon, 2015-02-16 at 17:56 +0700, Arseny Solokha wrote:
quoted
Drop unused fsl_mpic_primary_get_version(), mpic_set_clk_ratio(),
mpic_set_serial_int().
I'm always happy to remove unused code, but the interesting question is why are
they unused? Please tell me in the changelog.
To being able to give a definitive answer, it's necessary to understand
the intentions of original developers of these pieces. I just can tell
these functions have no users and trivial grepping easily proves it;
I've got the impression they are here only for the sake of
implementation completeness.
Yeah OK. I didn't expect you to read the minds of the developers who wrote the
code :)
Two machines at hands, e300 and e500 based, boot and run without
regressions on my workload with this series applied. The removed code
seems also been rarely touched, so it seems the series is safe at least
in general. But I can't obviously express any strong point in support of
the series, so it's completely OK to leave things as is.
OK that's a good data point.
+ fsl_mpic_primary_get_version() is just a safe wrapper around
fsl_mpic_get_version() for SMP configurations. While the latter is
called explicitly for handling PIC initialization and setting up error
interrupt vector depending on PIC hardware version, the former isn't
used for anything.
+ As for mpic_set_clk_ratio() and mpic_set_serial_int(), they both
are almost nine years old[1] but still have no chance to be called even
from out-of-tree modules because they both are __init and of course
aren't exported. Non-demanded functionality?
Of course I'll include the last two paragraphs into the V2 patch
description if the explanation is convincing enough and you ACK it. If
the patch is safe it's also necessary to extend it a bit, making its
second part actually a complete revert of [1].
[1] https://lists.ozlabs.org/pipermail/linuxppc-dev/2006-June/023867.html
That is more like what I was looking for.
If I just get a patch saying "removed unused foo()", I have to go and dig and
find out:
- was it recently added and will be used soon?
- is it ancient and never used, if so can we work out why, ie. feature X
never landed so this code is no longer needed.
- is it old code that *was* used but isn't now because commit ... removed the
last user.
- is it code that *should* be used, but isn't for some odd reason?
So if you can provide that sort of detail for me, that really adds value to the
patch. Otherwise the patch is basically just a TODO for me, to go and work out
why the code is unused.
cheers
On Thu, 2015-02-19 at 19:26 +0700, Arseny Solokha wrote:
+ fsl_mpic_primary_get_version() is just a safe wrapper around
fsl_mpic_get_version() for SMP configurations. While the latter is
called explicitly for handling PIC initialization and setting up error
interrupt vector depending on PIC hardware version, the former isn't
used for anything.
It was meant to be used by http://patchwork.ozlabs.org/patch/233211/
which never got respun. Hongtao, do you plan to revisit that patch?
-Scott
SGkgU2NvdHQsDQoNCkknbSByZWFsbHkgc29ycnkgZm9yIGxlYXZlIHRoaXMgcGF0Y2ggbGlrZSBh
IHpvbWJpZS4NCk5vdyBJIGhhdmUgcGxhbiB0byByZXZpc2l0IHRoaXMgcGF0Y2guDQoNCkZyb20g
dGhlIHByZXZpb3VzIGNvbW1lbnRzIHRoZSBjb21waWxlIGVycm9yIHdhcyBmaXhlZC4NCkJ1dCBi
ZXlvbmQgdGhhdCBJIGhhdmUgaGFkIG5vIHBsYW4gdG8gdXBkYXRlIGl0Lg0KDQpDb3VsZCB5b3Ug
cGxlYXNlIGNvbW1lbnQgb24gd2h5IGl0J3Mgc3RpbGwgb24gaG9sZD8NCg0KVGhhbmtzLg0KDQoN
Cj4gLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4gRnJvbTogV29vZCBTY290dC1CMDc0MjEN
Cj4gU2VudDogVHVlc2RheSwgRmVicnVhcnkgMjQsIDIwMTUgNTozMiBBTQ0KPiBUbzogQXJzZW55
IFNvbG9raGENCj4gQ2M6IE1pY2hhZWwgRWxsZXJtYW47IEJlbmphbWluIEhlcnJlbnNjaG1pZHQ7
IFBhdWwgTWFja2VycmFzOyBsaW51eHBwYy0NCj4gZGV2QGxpc3RzLm96bGFicy5vcmc7IGxpbnV4
LWtlcm5lbEB2Z2VyLmtlcm5lbC5vcmc7IEppYSBIb25ndGFvLUIzODk1MQ0KPiBTdWJqZWN0OiBS
ZTogW1BBVENIIDQvNF0gcG93ZXJwYy9tcGljOiByZW1vdmUgdW51c2VkIGZ1bmN0aW9ucw0KPiAN
Cj4gT24gVGh1LCAyMDE1LTAyLTE5IGF0IDE5OjI2ICswNzAwLCBBcnNlbnkgU29sb2toYSB3cm90
ZToNCj4gPiAgICsgZnNsX21waWNfcHJpbWFyeV9nZXRfdmVyc2lvbigpIGlzIGp1c3QgYSBzYWZl
IHdyYXBwZXIgYXJvdW5kDQo+ID4gZnNsX21waWNfZ2V0X3ZlcnNpb24oKSBmb3IgU01QIGNvbmZp
Z3VyYXRpb25zLiBXaGlsZSB0aGUgbGF0dGVyIGlzDQo+ID4gY2FsbGVkIGV4cGxpY2l0bHkgZm9y
IGhhbmRsaW5nIFBJQyBpbml0aWFsaXphdGlvbiBhbmQgc2V0dGluZyB1cCBlcnJvcg0KPiA+IGlu
dGVycnVwdCB2ZWN0b3IgZGVwZW5kaW5nIG9uIFBJQyBoYXJkd2FyZSB2ZXJzaW9uLCB0aGUgZm9y
bWVyIGlzbid0DQo+ID4gdXNlZCBmb3IgYW55dGhpbmcuDQo+IA0KPiBJdCB3YXMgbWVhbnQgdG8g
YmUgdXNlZCBieSBodHRwOi8vcGF0Y2h3b3JrLm96bGFicy5vcmcvcGF0Y2gvMjMzMjExLw0KPiB3
aGljaCBuZXZlciBnb3QgcmVzcHVuLiAgSG9uZ3RhbywgZG8geW91IHBsYW4gdG8gcmV2aXNpdCB0
aGF0IHBhdGNoPw0KPiANCj4gLVNjb3R0DQo+IA0KDQo=
On Wed, 2015-02-25 at 20:39 -0600, Jia Hongtao-B38951 wrote:
Hi Scott,
I'm really sorry for leave this patch like a zombie.
Now I have plan to revisit this patch.
From the previous comments the compile error was fixed.
But beyond that I have had no plan to update it.
Could you please comment on why it's still on hold?
Kumar had some comments.
-Scott