| Message ID | 158128306.gX9FjqECDu@vostro.rjw.lan (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
Hi Rafael, On 4 February 2015 at 08:48, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > On Tuesday, February 03, 2015 09:44:36 PM Cristian wrote: >> 2015-02-03 12:11 GMT-03:00 Rafael J. Wysocki <rjw@rjwysocki.net>: >> > On Tuesday, February 03, 2015 10:40:00 AM Cristian wrote: >> >> Hello, >> >> >> >> linux 3.19-rc7 >> >> Ubuntu Vivid 14.04 alpha >> >> >> >> dmesg: >> >> [ 1.293820] ACPI PCC probe failed. >> >> >> >> https://bugzilla.kernel.org/show_bug.cgi?id=92551 >> > >> > Is that a functional problem or just the message? >> > >> > >> > -- >> > I speak only for myself. >> > Rafael J. Wysocki, Intel Open Source Technology Center. >> >> Apparently one message. > > So it looks like you build the PCC mailbox driver which is new in 3.19-rc and > that driver fails to load, because it doesn't find hardware to work with. > > The message is harmless, but it also is not useful. The driver in question > seems to be overly verbose to me in general. Apologies. Looks like leftover from some early "printk" style debugging. :) > > The patch below should make the message go away unless the printing of debug > messages is on. > > Ashwin, that whole thing requires cleaning up: > - It prints uninteresting debug messages with KERN_ERR or warning priority in > *many* places. > - The error codes from acpi_pcc_probe() are ignored, so why bother to return > any error codes from there? > - If platform_create_bundle() fails, the debug message doesn't tell us the > reason, so why bother to print it? > > I'm not going to consider any users of this for merging before that cleanup > happens. In V4 of the CPPC patchset I've simplified the PCC code a lot which should make many of the prints go away. Can we consider that patch (I'll add any more pr_debugs to it) or would you prefer having a separate cleanup patch for this? Thanks, Ashwin > > Rafael > > > --- > drivers/mailbox/pcc.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > Index: linux-pm/drivers/mailbox/pcc.c > =================================================================== > --- linux-pm.orig/drivers/mailbox/pcc.c > +++ linux-pm/drivers/mailbox/pcc.c > @@ -386,7 +386,7 @@ static int __init pcc_init(void) > ret = acpi_pcc_probe(); > > if (ret) { > - pr_err("ACPI PCC probe failed.\n"); > + pr_debug("ACPI PCC probe failed.\n"); > return -ENODEV; > } > > @@ -394,7 +394,7 @@ static int __init pcc_init(void) > pcc_mbox_probe, NULL, 0, NULL, 0); > > if (!pcc_pdev) { > - pr_err("Err creating PCC platform bundle\n"); > + pr_debug("Err creating PCC platform bundle\n"); > return -ENODEV; > } > > -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 4 February 2015 at 17:14, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > On Wednesday, February 04, 2015 09:56:20 AM Ashwin Chaugule wrote: >> Hi Rafael, >> >> On 4 February 2015 at 08:48, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: >> > On Tuesday, February 03, 2015 09:44:36 PM Cristian wrote: >> >> 2015-02-03 12:11 GMT-03:00 Rafael J. Wysocki <rjw@rjwysocki.net>: >> >> > On Tuesday, February 03, 2015 10:40:00 AM Cristian wrote: >> > >> > So it looks like you build the PCC mailbox driver which is new in 3.19-rc and >> > that driver fails to load, because it doesn't find hardware to work with. >> > >> > The message is harmless, but it also is not useful. The driver in question >> > seems to be overly verbose to me in general. >> >> Apologies. Looks like leftover from some early "printk" style debugging. :) > > Well, does that mean I should apply the patch below? Sure. Thanks. > >> > The patch below should make the message go away unless the printing of debug >> > messages is on. >> > >> > Ashwin, that whole thing requires cleaning up: >> > - It prints uninteresting debug messages with KERN_ERR or warning priority in >> > *many* places. >> > - The error codes from acpi_pcc_probe() are ignored, so why bother to return >> > any error codes from there? >> > - If platform_create_bundle() fails, the debug message doesn't tell us the >> > reason, so why bother to print it? >> > >> > I'm not going to consider any users of this for merging before that cleanup >> > happens. >> >> In V4 of the CPPC patchset I've simplified the PCC code a lot which >> should make many of the prints go away. Can we consider that patch >> (I'll add any more pr_debugs to it) or would you prefer having a >> separate cleanup patch for this? > > This is a patch for the mailbox subsystem maintainer to consider. If it is > accepted and if there are any of these excessvely verbose messages still present, > I'll expect them to be fixed in a separate patch. Ok. > > I have one more concern about this driver. Namely, what benefit is there to > people like Cristian from it at all? Its of use only if they have a PCC client (MPST, CPPC, RAS) driver. Looks like PCC was explicitly enabled in this kernel. config PCC bool "Platform Communication Channel Driver" depends on ACPI help ACPI 5.0+ spec defines a generic mode of communication between the OS and a platform such as the BMC. This medium (PCC) is typically used by CPPC (ACPI CPU Performance management), RAS (ACPI reliability protocol) and MPST (ACPI Memory power states). Select this driver if your platform implements the PCC clients mentioned above. Thanks, Ashwin -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday, February 04, 2015 09:56:20 AM Ashwin Chaugule wrote: > Hi Rafael, > > On 4 February 2015 at 08:48, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > > On Tuesday, February 03, 2015 09:44:36 PM Cristian wrote: > >> 2015-02-03 12:11 GMT-03:00 Rafael J. Wysocki <rjw@rjwysocki.net>: > >> > On Tuesday, February 03, 2015 10:40:00 AM Cristian wrote: > >> >> Hello, > >> >> > >> >> linux 3.19-rc7 > >> >> Ubuntu Vivid 14.04 alpha > >> >> > >> >> dmesg: > >> >> [ 1.293820] ACPI PCC probe failed. > >> >> > >> >> https://bugzilla.kernel.org/show_bug.cgi?id=92551 > >> > > >> > Is that a functional problem or just the message? > >> > > >> > > >> > -- > >> > I speak only for myself. > >> > Rafael J. Wysocki, Intel Open Source Technology Center. > >> > >> Apparently one message. > > > > So it looks like you build the PCC mailbox driver which is new in 3.19-rc and > > that driver fails to load, because it doesn't find hardware to work with. > > > > The message is harmless, but it also is not useful. The driver in question > > seems to be overly verbose to me in general. > > Apologies. Looks like leftover from some early "printk" style debugging. :) Well, does that mean I should apply the patch below? > > The patch below should make the message go away unless the printing of debug > > messages is on. > > > > Ashwin, that whole thing requires cleaning up: > > - It prints uninteresting debug messages with KERN_ERR or warning priority in > > *many* places. > > - The error codes from acpi_pcc_probe() are ignored, so why bother to return > > any error codes from there? > > - If platform_create_bundle() fails, the debug message doesn't tell us the > > reason, so why bother to print it? > > > > I'm not going to consider any users of this for merging before that cleanup > > happens. > > In V4 of the CPPC patchset I've simplified the PCC code a lot which > should make many of the prints go away. Can we consider that patch > (I'll add any more pr_debugs to it) or would you prefer having a > separate cleanup patch for this? This is a patch for the mailbox subsystem maintainer to consider. If it is accepted and if there are any of these excessvely verbose messages still present, I'll expect them to be fixed in a separate patch. I have one more concern about this driver. Namely, what benefit is there to people like Cristian from it at all? > > --- > > drivers/mailbox/pcc.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > Index: linux-pm/drivers/mailbox/pcc.c > > =================================================================== > > --- linux-pm.orig/drivers/mailbox/pcc.c > > +++ linux-pm/drivers/mailbox/pcc.c > > @@ -386,7 +386,7 @@ static int __init pcc_init(void) > > ret = acpi_pcc_probe(); > > > > if (ret) { > > - pr_err("ACPI PCC probe failed.\n"); > > + pr_debug("ACPI PCC probe failed.\n"); > > return -ENODEV; > > } > > > > @@ -394,7 +394,7 @@ static int __init pcc_init(void) > > pcc_mbox_probe, NULL, 0, NULL, 0); > > > > if (!pcc_pdev) { > > - pr_err("Err creating PCC platform bundle\n"); > > + pr_debug("Err creating PCC platform bundle\n"); > > return -ENODEV; > > } > > > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-acpi" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday, February 04, 2015 05:06:26 PM Ashwin Chaugule wrote: > On 4 February 2015 at 17:14, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > > On Wednesday, February 04, 2015 09:56:20 AM Ashwin Chaugule wrote: > >> Hi Rafael, > >> > >> On 4 February 2015 at 08:48, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > >> > On Tuesday, February 03, 2015 09:44:36 PM Cristian wrote: > >> >> 2015-02-03 12:11 GMT-03:00 Rafael J. Wysocki <rjw@rjwysocki.net>: > >> >> > On Tuesday, February 03, 2015 10:40:00 AM Cristian wrote: > >> > > >> > So it looks like you build the PCC mailbox driver which is new in 3.19-rc and > >> > that driver fails to load, because it doesn't find hardware to work with. > >> > > >> > The message is harmless, but it also is not useful. The driver in question > >> > seems to be overly verbose to me in general. > >> > >> Apologies. Looks like leftover from some early "printk" style debugging. :) > > > > Well, does that mean I should apply the patch below? > > Sure. Thanks. > > > > >> > The patch below should make the message go away unless the printing of debug > >> > messages is on. > >> > > >> > Ashwin, that whole thing requires cleaning up: > >> > - It prints uninteresting debug messages with KERN_ERR or warning priority in > >> > *many* places. > >> > - The error codes from acpi_pcc_probe() are ignored, so why bother to return > >> > any error codes from there? > >> > - If platform_create_bundle() fails, the debug message doesn't tell us the > >> > reason, so why bother to print it? > >> > > >> > I'm not going to consider any users of this for merging before that cleanup > >> > happens. > >> > >> In V4 of the CPPC patchset I've simplified the PCC code a lot which > >> should make many of the prints go away. Can we consider that patch > >> (I'll add any more pr_debugs to it) or would you prefer having a > >> separate cleanup patch for this? > > > > This is a patch for the mailbox subsystem maintainer to consider. If it is > > accepted and if there are any of these excessvely verbose messages still present, > > I'll expect them to be fixed in a separate patch. > > Ok. > > > > > I have one more concern about this driver. Namely, what benefit is there to > > people like Cristian from it at all? > > Its of use only if they have a PCC client (MPST, CPPC, RAS) driver. > Looks like PCC was explicitly enabled in this kernel. > > config PCC > bool "Platform Communication Channel Driver" > depends on ACPI Can we make it depend on the clients instead and be set automatically when at least one of the clients is enabled? Otherwise distros will have a problem with deciding whether or not they should enable this driver and most of them will end up enabling it.
On 4 February 2015 at 18:04, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > On Wednesday, February 04, 2015 05:06:26 PM Ashwin Chaugule wrote: >> > I have one more concern about this driver. Namely, what benefit is there to >> > people like Cristian from it at all? >> >> Its of use only if they have a PCC client (MPST, CPPC, RAS) driver. >> Looks like PCC was explicitly enabled in this kernel. >> >> config PCC >> bool "Platform Communication Channel Driver" >> depends on ACPI > > Can we make it depend on the clients instead and be set automatically > when at least one of the clients is enabled? > > Otherwise distros will have a problem with deciding whether or not they > should enable this driver and most of them will end up enabling it. I see your point, but I'm not aware of any upstreamed client as of yet. There might be folks using this driver internally though with other clients. In such a case, is there a way to keep PCC disabled until a client (e.g. CPPC) is upstreamed? Alternately, is it that bad to keep it the way it is, given that the driver wont do anything unless PCCT is detected in firmware and a PCC client explicitly uses its API? Thanks, Ashwin -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday, February 04, 2015 06:38:39 PM Ashwin Chaugule wrote: > On 4 February 2015 at 18:04, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > > On Wednesday, February 04, 2015 05:06:26 PM Ashwin Chaugule wrote: > >> > I have one more concern about this driver. Namely, what benefit is there to > >> > people like Cristian from it at all? > >> > >> Its of use only if they have a PCC client (MPST, CPPC, RAS) driver. > >> Looks like PCC was explicitly enabled in this kernel. > >> > >> config PCC > >> bool "Platform Communication Channel Driver" > >> depends on ACPI > > > > Can we make it depend on the clients instead and be set automatically > > when at least one of the clients is enabled? > > > > Otherwise distros will have a problem with deciding whether or not they > > should enable this driver and most of them will end up enabling it. > > I see your point, but I'm not aware of any upstreamed client as of > yet. There might be folks using this driver internally though with > other clients. In such a case, is there a way to keep PCC disabled > until a client (e.g. CPPC) is upstreamed? Make it depend on EXPERT or something like that until the first client is added and then make it depend on that client. :-) > Alternately, is it that bad to keep it the way it is, given that the > driver wont do anything unless PCCT is detected in firmware and a PCC > client explicitly uses its API? Well, is it really useful this way?
Hi Rafael, On 4 February 2015 at 20:28, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > On Wednesday, February 04, 2015 06:38:39 PM Ashwin Chaugule wrote: >> On 4 February 2015 at 18:04, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: >> > On Wednesday, February 04, 2015 05:06:26 PM Ashwin Chaugule wrote: >> >> > I have one more concern about this driver. Namely, what benefit is there to >> >> > people like Cristian from it at all? >> >> >> >> Its of use only if they have a PCC client (MPST, CPPC, RAS) driver. >> >> Looks like PCC was explicitly enabled in this kernel. >> >> >> >> config PCC >> >> bool "Platform Communication Channel Driver" >> >> depends on ACPI >> > >> > Can we make it depend on the clients instead and be set automatically >> > when at least one of the clients is enabled? >> > >> > Otherwise distros will have a problem with deciding whether or not they >> > should enable this driver and most of them will end up enabling it. >> >> I see your point, but I'm not aware of any upstreamed client as of >> yet. There might be folks using this driver internally though with >> other clients. In such a case, is there a way to keep PCC disabled >> until a client (e.g. CPPC) is upstreamed? > > Make it depend on EXPERT or something like that until the first client is > added and then make it depend on that client. :-) Doesn't sound too bad. Hope that doesn't end up enabling unintended options in the kernel which aren't exposed in kconfig. FWIW, I think its better to make it depend on CPPC as part of the PCC cleanup patch that I have going in the CPPC patchset. > >> Alternately, is it that bad to keep it the way it is, given that the >> driver wont do anything unless PCCT is detected in firmware and a PCC >> client explicitly uses its API? > > Well, is it really useful this way? Not really, but not particularly harmful either. :) Besides, it seems like for the sake of genericness, distros enable several options that are irrelevant to a platform. In this case, PCC would be harmlessly enabled only until CPPC makes its way upstream (which is actively being worked on anyway). Cheers, Ashwin. -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thursday, February 05, 2015 10:44:13 AM Ashwin Chaugule wrote: > Hi Rafael, > > On 4 February 2015 at 20:28, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > > On Wednesday, February 04, 2015 06:38:39 PM Ashwin Chaugule wrote: > >> On 4 February 2015 at 18:04, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > >> > On Wednesday, February 04, 2015 05:06:26 PM Ashwin Chaugule wrote: > >> >> > I have one more concern about this driver. Namely, what benefit is there to > >> >> > people like Cristian from it at all? > >> >> > >> >> Its of use only if they have a PCC client (MPST, CPPC, RAS) driver. > >> >> Looks like PCC was explicitly enabled in this kernel. > >> >> > >> >> config PCC > >> >> bool "Platform Communication Channel Driver" > >> >> depends on ACPI > >> > > >> > Can we make it depend on the clients instead and be set automatically > >> > when at least one of the clients is enabled? > >> > > >> > Otherwise distros will have a problem with deciding whether or not they > >> > should enable this driver and most of them will end up enabling it. > >> > >> I see your point, but I'm not aware of any upstreamed client as of > >> yet. There might be folks using this driver internally though with > >> other clients. In such a case, is there a way to keep PCC disabled > >> until a client (e.g. CPPC) is upstreamed? > > > > Make it depend on EXPERT or something like that until the first client is > > added and then make it depend on that client. :-) > > Doesn't sound too bad. Hope that doesn't end up enabling unintended > options in the kernel which aren't exposed in kconfig. > FWIW, I think its better to make it depend on CPPC as part of the PCC > cleanup patch that I have going in the CPPC patchset. That's fine by me. > >> Alternately, is it that bad to keep it the way it is, given that the > >> driver wont do anything unless PCCT is detected in firmware and a PCC > >> client explicitly uses its API? > > > > Well, is it really useful this way? > > Not really, but not particularly harmful either. :) Besides, it seems > like for the sake of genericness, distros enable several options that > are irrelevant to a platform. In this case, PCC would be harmlessly > enabled only until CPPC makes its way upstream (which is actively > being worked on anyway). For one, I don't like it being easy to enable by mistake on x86 where it is not useful at all (and won't be at least for some time).
Hello, On 5 February 2015 at 19:49, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > On Thursday, February 05, 2015 10:44:13 AM Ashwin Chaugule wrote: >> Hi Rafael, >> On 4 February 2015 at 20:28, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: >> > On Wednesday, February 04, 2015 06:38:39 PM Ashwin Chaugule wrote: >> >> On 4 February 2015 at 18:04, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: >> >> > On Wednesday, February 04, 2015 05:06:26 PM Ashwin Chaugule wrote: >> >> >> > I have one more concern about this driver. Namely, what benefit is there to >> >> >> > people like Cristian from it at all? >> >> >> >> >> >> Its of use only if they have a PCC client (MPST, CPPC, RAS) driver. >> >> >> Looks like PCC was explicitly enabled in this kernel. >> >> >> >> >> >> config PCC >> >> >> bool "Platform Communication Channel Driver" >> >> >> depends on ACPI >> >> > >> >> > Can we make it depend on the clients instead and be set automatically >> >> > when at least one of the clients is enabled? >> >> > >> >> > Otherwise distros will have a problem with deciding whether or not they >> >> > should enable this driver and most of them will end up enabling it. >> >> >> >> I see your point, but I'm not aware of any upstreamed client as of >> >> yet. There might be folks using this driver internally though with >> >> other clients. In such a case, is there a way to keep PCC disabled >> >> until a client (e.g. CPPC) is upstreamed? >> > >> > Make it depend on EXPERT or something like that until the first client is >> > added and then make it depend on that client. :-) >> >> Doesn't sound too bad. Hope that doesn't end up enabling unintended >> options in the kernel which aren't exposed in kconfig. >> FWIW, I think its better to make it depend on CPPC as part of the PCC >> cleanup patch that I have going in the CPPC patchset. > > That's fine by me. Thanks. > >> >> Alternately, is it that bad to keep it the way it is, given that the >> >> driver wont do anything unless PCCT is detected in firmware and a PCC >> >> client explicitly uses its API? >> > >> > Well, is it really useful this way? >> >> Not really, but not particularly harmful either. :) Besides, it seems >> like for the sake of genericness, distros enable several options that >> are irrelevant to a platform. In this case, PCC would be harmlessly >> enabled only until CPPC makes its way upstream (which is actively >> being worked on anyway). > > For one, I don't like it being easy to enable by mistake on x86 where it is > not useful at all (and won't be at least for some time). I hear you. I knew about the trend for CPPC on x86, wasn't sure about MPST and RAS. :) Anyway, this shouldnt be an issue anymore with the approach mentioned above for CPPC on ARM64. Cheers, Ashwin. -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Index: linux-pm/drivers/mailbox/pcc.c =================================================================== --- linux-pm.orig/drivers/mailbox/pcc.c +++ linux-pm/drivers/mailbox/pcc.c @@ -386,7 +386,7 @@ static int __init pcc_init(void) ret = acpi_pcc_probe(); if (ret) { - pr_err("ACPI PCC probe failed.\n"); + pr_debug("ACPI PCC probe failed.\n"); return -ENODEV; } @@ -394,7 +394,7 @@ static int __init pcc_init(void) pcc_mbox_probe, NULL, 0, NULL, 0); if (!pcc_pdev) { - pr_err("Err creating PCC platform bundle\n"); + pr_debug("Err creating PCC platform bundle\n"); return -ENODEV; }