* [PATCH v6 01/17] EDAC/device: Respect any driver-supplied workqueue polling value
[not found] <20230118150904.26913-1-manivannan.sadhasivam@linaro.org>
@ 2023-01-18 15:08 ` Manivannan Sadhasivam
2023-01-18 17:46 ` Bjorn Andersson
2023-01-19 10:55 ` Borislav Petkov
2023-01-18 15:08 ` [PATCH v6 03/17] EDAC/qcom: Do not pass llcc_driv_data as edac_device_ctl_info's pvt_info Manivannan Sadhasivam
2023-01-18 15:09 ` [PATCH v6 17/17] soc: qcom: llcc: Do not create EDAC platform device on SDM845 Manivannan Sadhasivam
2 siblings, 2 replies; 12+ messages in thread
From: Manivannan Sadhasivam @ 2023-01-18 15:08 UTC (permalink / raw)
To: andersson, robh+dt, krzysztof.kozlowski+dt, bp, tony.luck
Cc: quic_saipraka, konrad.dybcio, linux-arm-msm, linux-kernel,
james.morse, mchehab, rric, linux-edac, quic_ppareek, luca.weiss,
ahalaney, steev, Manivannan Sadhasivam, stable
The EDAC drivers may optionally pass the poll_msec value. Use that value
if available, else fall back to 1000ms.
[ bp: Touchups. ]
Fixes: e27e3dac6517 ("drivers/edac: add edac_device class")
Reported-by: Luca Weiss <luca.weiss@fairphone.com>
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de>
Tested-by: Steev Klimaszewski <steev@kali.org> # Thinkpad X13s
Tested-by: Andrew Halaney <ahalaney@redhat.com> # sa8540p-ride
Cc: <stable@vger.kernel.org> # 4.9
Link: https://lore.kernel.org/r/COZYL8MWN97H.MROQ391BGA09@otso
---
drivers/edac/edac_device.c | 15 +++++++--------
1 file changed, 7 insertions(+), 8 deletions(-)
diff --git a/drivers/edac/edac_device.c b/drivers/edac/edac_device.c
index 19522c568aa5..a50b7bcfb731 100644
--- a/drivers/edac/edac_device.c
+++ b/drivers/edac/edac_device.c
@@ -34,6 +34,9 @@
static DEFINE_MUTEX(device_ctls_mutex);
static LIST_HEAD(edac_device_list);
+/* Default workqueue processing interval on this instance, in msecs */
+#define DEFAULT_POLL_INTERVAL 1000
+
#ifdef CONFIG_EDAC_DEBUG
static void edac_device_dump_device(struct edac_device_ctl_info *edac_dev)
{
@@ -336,7 +339,7 @@ static void edac_device_workq_function(struct work_struct *work_req)
* whole one second to save timers firing all over the period
* between integral seconds
*/
- if (edac_dev->poll_msec == 1000)
+ if (edac_dev->poll_msec == DEFAULT_POLL_INTERVAL)
edac_queue_work(&edac_dev->work, round_jiffies_relative(edac_dev->delay));
else
edac_queue_work(&edac_dev->work, edac_dev->delay);
@@ -366,7 +369,7 @@ static void edac_device_workq_setup(struct edac_device_ctl_info *edac_dev,
* timers firing on sub-second basis, while they are happy
* to fire together on the 1 second exactly
*/
- if (edac_dev->poll_msec == 1000)
+ if (edac_dev->poll_msec == DEFAULT_POLL_INTERVAL)
edac_queue_work(&edac_dev->work, round_jiffies_relative(edac_dev->delay));
else
edac_queue_work(&edac_dev->work, edac_dev->delay);
@@ -398,7 +401,7 @@ void edac_device_reset_delay_period(struct edac_device_ctl_info *edac_dev,
{
unsigned long jiffs = msecs_to_jiffies(value);
- if (value == 1000)
+ if (value == DEFAULT_POLL_INTERVAL)
jiffs = round_jiffies_relative(value);
edac_dev->poll_msec = value;
@@ -443,11 +446,7 @@ int edac_device_add_device(struct edac_device_ctl_info *edac_dev)
/* This instance is NOW RUNNING */
edac_dev->op_state = OP_RUNNING_POLL;
- /*
- * enable workq processing on this instance,
- * default = 1000 msec
- */
- edac_device_workq_setup(edac_dev, 1000);
+ edac_device_workq_setup(edac_dev, edac_dev->poll_msec ?: DEFAULT_POLL_INTERVAL);
} else {
edac_dev->op_state = OP_RUNNING_INTERRUPT;
}
--
2.25.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v6 03/17] EDAC/qcom: Do not pass llcc_driv_data as edac_device_ctl_info's pvt_info
[not found] <20230118150904.26913-1-manivannan.sadhasivam@linaro.org>
2023-01-18 15:08 ` [PATCH v6 01/17] EDAC/device: Respect any driver-supplied workqueue polling value Manivannan Sadhasivam
@ 2023-01-18 15:08 ` Manivannan Sadhasivam
2023-01-20 18:56 ` Borislav Petkov
2023-01-18 15:09 ` [PATCH v6 17/17] soc: qcom: llcc: Do not create EDAC platform device on SDM845 Manivannan Sadhasivam
2 siblings, 1 reply; 12+ messages in thread
From: Manivannan Sadhasivam @ 2023-01-18 15:08 UTC (permalink / raw)
To: andersson, robh+dt, krzysztof.kozlowski+dt, bp, tony.luck
Cc: quic_saipraka, konrad.dybcio, linux-arm-msm, linux-kernel,
james.morse, mchehab, rric, linux-edac, quic_ppareek, luca.weiss,
ahalaney, steev, Manivannan Sadhasivam, stable
The memory for "llcc_driv_data" is allocated by the LLCC driver. But when
it is passed as "pvt_info" to the EDAC core, it will get freed during the
qcom_edac driver release. So when the qcom_edac driver gets probed again,
it will try to use the freed data leading to the use-after-free bug.
Hence, do not pass "llcc_driv_data" as pvt_info but rather reference it
using the "platform_data" in the qcom_edac driver.
Cc: <stable@vger.kernel.org> # 4.20
Fixes: 27450653f1db ("drivers: edac: Add EDAC driver support for QCOM SoCs")
Tested-by: Steev Klimaszewski <steev@kali.org> # Thinkpad X13s
Tested-by: Andrew Halaney <ahalaney@redhat.com> # sa8540p-ride
Reported-by: Steev Klimaszewski <steev@kali.org>
Reviewed-by: Borislav Petkov (AMD) <bp@alien8.de>
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
drivers/edac/qcom_edac.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/edac/qcom_edac.c b/drivers/edac/qcom_edac.c
index 9e77fa84e84f..3256254c3722 100644
--- a/drivers/edac/qcom_edac.c
+++ b/drivers/edac/qcom_edac.c
@@ -252,7 +252,7 @@ dump_syn_reg_values(struct llcc_drv_data *drv, u32 bank, int err_type)
static int
dump_syn_reg(struct edac_device_ctl_info *edev_ctl, int err_type, u32 bank)
{
- struct llcc_drv_data *drv = edev_ctl->pvt_info;
+ struct llcc_drv_data *drv = edev_ctl->dev->platform_data;
int ret;
ret = dump_syn_reg_values(drv, bank, err_type);
@@ -289,7 +289,7 @@ static irqreturn_t
llcc_ecc_irq_handler(int irq, void *edev_ctl)
{
struct edac_device_ctl_info *edac_dev_ctl = edev_ctl;
- struct llcc_drv_data *drv = edac_dev_ctl->pvt_info;
+ struct llcc_drv_data *drv = edac_dev_ctl->dev->platform_data;
irqreturn_t irq_rc = IRQ_NONE;
u32 drp_error, trp_error, i;
int ret;
@@ -358,7 +358,6 @@ static int qcom_llcc_edac_probe(struct platform_device *pdev)
edev_ctl->dev_name = dev_name(dev);
edev_ctl->ctl_name = "llcc";
edev_ctl->panic_on_ue = LLCC_ERP_PANIC_ON_UE;
- edev_ctl->pvt_info = llcc_driv_data;
rc = edac_device_add_device(edev_ctl);
if (rc)
--
2.25.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v6 17/17] soc: qcom: llcc: Do not create EDAC platform device on SDM845
[not found] <20230118150904.26913-1-manivannan.sadhasivam@linaro.org>
2023-01-18 15:08 ` [PATCH v6 01/17] EDAC/device: Respect any driver-supplied workqueue polling value Manivannan Sadhasivam
2023-01-18 15:08 ` [PATCH v6 03/17] EDAC/qcom: Do not pass llcc_driv_data as edac_device_ctl_info's pvt_info Manivannan Sadhasivam
@ 2023-01-18 15:09 ` Manivannan Sadhasivam
2023-01-18 15:37 ` Krzysztof Kozlowski
2 siblings, 1 reply; 12+ messages in thread
From: Manivannan Sadhasivam @ 2023-01-18 15:09 UTC (permalink / raw)
To: andersson, robh+dt, krzysztof.kozlowski+dt, bp, tony.luck
Cc: quic_saipraka, konrad.dybcio, linux-arm-msm, linux-kernel,
james.morse, mchehab, rric, linux-edac, quic_ppareek, luca.weiss,
ahalaney, steev, Manivannan Sadhasivam, stable
The platforms based on SDM845 SoC locks the access to EDAC registers in the
bootloader. So probing the EDAC driver will result in a crash. Hence,
disable the creation of EDAC platform device on all SDM845 devices.
The issue has been observed on Lenovo Yoga C630 and DB845c.
Cc: <stable@vger.kernel.org> # 5.10
Reported-by: Steev Klimaszewski <steev@kali.org>
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
drivers/soc/qcom/llcc-qcom.c | 17 ++++++++++++-----
1 file changed, 12 insertions(+), 5 deletions(-)
diff --git a/drivers/soc/qcom/llcc-qcom.c b/drivers/soc/qcom/llcc-qcom.c
index 7b7c5a38bac6..8d840702df50 100644
--- a/drivers/soc/qcom/llcc-qcom.c
+++ b/drivers/soc/qcom/llcc-qcom.c
@@ -1012,11 +1012,18 @@ static int qcom_llcc_probe(struct platform_device *pdev)
drv_data->ecc_irq = platform_get_irq_optional(pdev, 0);
- llcc_edac = platform_device_register_data(&pdev->dev,
- "qcom_llcc_edac", -1, drv_data,
- sizeof(*drv_data));
- if (IS_ERR(llcc_edac))
- dev_err(dev, "Failed to register llcc edac driver\n");
+ /*
+ * The platforms based on SDM845 SoC locks the access to EDAC registers
+ * in bootloader. So probing the EDAC driver will result in a crash.
+ * Hence, disable the creation of EDAC platform device on SDM845.
+ */
+ if (!of_device_is_compatible(dev->of_node, "qcom,sdm845-llcc")) {
+ llcc_edac = platform_device_register_data(&pdev->dev,
+ "qcom_llcc_edac", -1, drv_data,
+ sizeof(*drv_data));
+ if (IS_ERR(llcc_edac))
+ dev_err(dev, "Failed to register llcc edac driver\n");
+ }
return 0;
err:
--
2.25.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v6 17/17] soc: qcom: llcc: Do not create EDAC platform device on SDM845
2023-01-18 15:09 ` [PATCH v6 17/17] soc: qcom: llcc: Do not create EDAC platform device on SDM845 Manivannan Sadhasivam
@ 2023-01-18 15:37 ` Krzysztof Kozlowski
2023-01-18 15:59 ` Manivannan Sadhasivam
0 siblings, 1 reply; 12+ messages in thread
From: Krzysztof Kozlowski @ 2023-01-18 15:37 UTC (permalink / raw)
To: Manivannan Sadhasivam, andersson, robh+dt, krzysztof.kozlowski+dt,
bp, tony.luck
Cc: quic_saipraka, konrad.dybcio, linux-arm-msm, linux-kernel,
james.morse, mchehab, rric, linux-edac, quic_ppareek, luca.weiss,
ahalaney, steev, stable
On 18/01/2023 16:09, Manivannan Sadhasivam wrote:
> The platforms based on SDM845 SoC locks the access to EDAC registers in the
> bootloader. So probing the EDAC driver will result in a crash. Hence,
> disable the creation of EDAC platform device on all SDM845 devices.
>
> The issue has been observed on Lenovo Yoga C630 and DB845c.
>
> Cc: <stable@vger.kernel.org> # 5.10
> Reported-by: Steev Klimaszewski <steev@kali.org>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
> drivers/soc/qcom/llcc-qcom.c | 17 ++++++++++++-----
> 1 file changed, 12 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/soc/qcom/llcc-qcom.c b/drivers/soc/qcom/llcc-qcom.c
> index 7b7c5a38bac6..8d840702df50 100644
> --- a/drivers/soc/qcom/llcc-qcom.c
> +++ b/drivers/soc/qcom/llcc-qcom.c
> @@ -1012,11 +1012,18 @@ static int qcom_llcc_probe(struct platform_device *pdev)
>
> drv_data->ecc_irq = platform_get_irq_optional(pdev, 0);
>
> - llcc_edac = platform_device_register_data(&pdev->dev,
> - "qcom_llcc_edac", -1, drv_data,
> - sizeof(*drv_data));
> - if (IS_ERR(llcc_edac))
> - dev_err(dev, "Failed to register llcc edac driver\n");
> + /*
> + * The platforms based on SDM845 SoC locks the access to EDAC registers
> + * in bootloader. So probing the EDAC driver will result in a crash.
> + * Hence, disable the creation of EDAC platform device on SDM845.
> + */
> + if (!of_device_is_compatible(dev->of_node, "qcom,sdm845-llcc")) {
Don't spread of_device_is_compatible() in driver code. You have driver
data for this.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v6 17/17] soc: qcom: llcc: Do not create EDAC platform device on SDM845
2023-01-18 15:37 ` Krzysztof Kozlowski
@ 2023-01-18 15:59 ` Manivannan Sadhasivam
2023-01-18 16:05 ` Krzysztof Kozlowski
0 siblings, 1 reply; 12+ messages in thread
From: Manivannan Sadhasivam @ 2023-01-18 15:59 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: andersson, robh+dt, krzysztof.kozlowski+dt, bp, tony.luck,
quic_saipraka, konrad.dybcio, linux-arm-msm, linux-kernel,
james.morse, mchehab, rric, linux-edac, quic_ppareek, luca.weiss,
ahalaney, steev, stable
On Wed, Jan 18, 2023 at 04:37:29PM +0100, Krzysztof Kozlowski wrote:
> On 18/01/2023 16:09, Manivannan Sadhasivam wrote:
> > The platforms based on SDM845 SoC locks the access to EDAC registers in the
> > bootloader. So probing the EDAC driver will result in a crash. Hence,
> > disable the creation of EDAC platform device on all SDM845 devices.
> >
> > The issue has been observed on Lenovo Yoga C630 and DB845c.
> >
> > Cc: <stable@vger.kernel.org> # 5.10
> > Reported-by: Steev Klimaszewski <steev@kali.org>
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > ---
> > drivers/soc/qcom/llcc-qcom.c | 17 ++++++++++++-----
> > 1 file changed, 12 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/soc/qcom/llcc-qcom.c b/drivers/soc/qcom/llcc-qcom.c
> > index 7b7c5a38bac6..8d840702df50 100644
> > --- a/drivers/soc/qcom/llcc-qcom.c
> > +++ b/drivers/soc/qcom/llcc-qcom.c
> > @@ -1012,11 +1012,18 @@ static int qcom_llcc_probe(struct platform_device *pdev)
> >
> > drv_data->ecc_irq = platform_get_irq_optional(pdev, 0);
> >
> > - llcc_edac = platform_device_register_data(&pdev->dev,
> > - "qcom_llcc_edac", -1, drv_data,
> > - sizeof(*drv_data));
> > - if (IS_ERR(llcc_edac))
> > - dev_err(dev, "Failed to register llcc edac driver\n");
> > + /*
> > + * The platforms based on SDM845 SoC locks the access to EDAC registers
> > + * in bootloader. So probing the EDAC driver will result in a crash.
> > + * Hence, disable the creation of EDAC platform device on SDM845.
> > + */
> > + if (!of_device_is_compatible(dev->of_node, "qcom,sdm845-llcc")) {
>
> Don't spread of_device_is_compatible() in driver code. You have driver
> data for this.
>
Yeah, but there is no ID to in the driver data to identify an SoC. I could add
one but is that really worth doing so? Is using of_device_is_compatible() in
drivers discouraged nowadays?
Thanks,
Mani
> Best regards,
> Krzysztof
>
--
மணிவண்ணன் சதாசிவம்
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v6 17/17] soc: qcom: llcc: Do not create EDAC platform device on SDM845
2023-01-18 15:59 ` Manivannan Sadhasivam
@ 2023-01-18 16:05 ` Krzysztof Kozlowski
2023-01-18 16:26 ` Manivannan Sadhasivam
0 siblings, 1 reply; 12+ messages in thread
From: Krzysztof Kozlowski @ 2023-01-18 16:05 UTC (permalink / raw)
To: Manivannan Sadhasivam
Cc: andersson, robh+dt, krzysztof.kozlowski+dt, bp, tony.luck,
quic_saipraka, konrad.dybcio, linux-arm-msm, linux-kernel,
james.morse, mchehab, rric, linux-edac, quic_ppareek, luca.weiss,
ahalaney, steev, stable
On 18/01/2023 16:59, Manivannan Sadhasivam wrote:
> On Wed, Jan 18, 2023 at 04:37:29PM +0100, Krzysztof Kozlowski wrote:
>> On 18/01/2023 16:09, Manivannan Sadhasivam wrote:
>>> The platforms based on SDM845 SoC locks the access to EDAC registers in the
>>> bootloader. So probing the EDAC driver will result in a crash. Hence,
>>> disable the creation of EDAC platform device on all SDM845 devices.
>>>
>>> The issue has been observed on Lenovo Yoga C630 and DB845c.
>>>
>>> Cc: <stable@vger.kernel.org> # 5.10
>>> Reported-by: Steev Klimaszewski <steev@kali.org>
>>> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
>>> ---
>>> drivers/soc/qcom/llcc-qcom.c | 17 ++++++++++++-----
>>> 1 file changed, 12 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/soc/qcom/llcc-qcom.c b/drivers/soc/qcom/llcc-qcom.c
>>> index 7b7c5a38bac6..8d840702df50 100644
>>> --- a/drivers/soc/qcom/llcc-qcom.c
>>> +++ b/drivers/soc/qcom/llcc-qcom.c
>>> @@ -1012,11 +1012,18 @@ static int qcom_llcc_probe(struct platform_device *pdev)
>>>
>>> drv_data->ecc_irq = platform_get_irq_optional(pdev, 0);
>>>
>>> - llcc_edac = platform_device_register_data(&pdev->dev,
>>> - "qcom_llcc_edac", -1, drv_data,
>>> - sizeof(*drv_data));
>>> - if (IS_ERR(llcc_edac))
>>> - dev_err(dev, "Failed to register llcc edac driver\n");
>>> + /*
>>> + * The platforms based on SDM845 SoC locks the access to EDAC registers
>>> + * in bootloader. So probing the EDAC driver will result in a crash.
>>> + * Hence, disable the creation of EDAC platform device on SDM845.
>>> + */
>>> + if (!of_device_is_compatible(dev->of_node, "qcom,sdm845-llcc")) {
>>
>> Don't spread of_device_is_compatible() in driver code. You have driver
>> data for this.
>>
>
> Yeah, but there is no ID to in the driver data to identify an SoC.
What do you mean there is no? You use exactly the same compatible as the
one in driver data.
> I could add
> one but is that really worth doing so? Is using of_device_is_compatible() in
> drivers discouraged nowadays?
Because it spreads variant matching all over. It does not scale. drv
data fields are the way or better quirks/flags.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v6 17/17] soc: qcom: llcc: Do not create EDAC platform device on SDM845
2023-01-18 16:05 ` Krzysztof Kozlowski
@ 2023-01-18 16:26 ` Manivannan Sadhasivam
2023-01-18 17:06 ` Krzysztof Kozlowski
0 siblings, 1 reply; 12+ messages in thread
From: Manivannan Sadhasivam @ 2023-01-18 16:26 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: andersson, robh+dt, krzysztof.kozlowski+dt, bp, tony.luck,
quic_saipraka, konrad.dybcio, linux-arm-msm, linux-kernel,
james.morse, mchehab, rric, linux-edac, quic_ppareek, luca.weiss,
ahalaney, steev, stable
On Wed, Jan 18, 2023 at 05:05:28PM +0100, Krzysztof Kozlowski wrote:
> On 18/01/2023 16:59, Manivannan Sadhasivam wrote:
> > On Wed, Jan 18, 2023 at 04:37:29PM +0100, Krzysztof Kozlowski wrote:
> >> On 18/01/2023 16:09, Manivannan Sadhasivam wrote:
> >>> The platforms based on SDM845 SoC locks the access to EDAC registers in the
> >>> bootloader. So probing the EDAC driver will result in a crash. Hence,
> >>> disable the creation of EDAC platform device on all SDM845 devices.
> >>>
> >>> The issue has been observed on Lenovo Yoga C630 and DB845c.
> >>>
> >>> Cc: <stable@vger.kernel.org> # 5.10
> >>> Reported-by: Steev Klimaszewski <steev@kali.org>
> >>> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> >>> ---
> >>> drivers/soc/qcom/llcc-qcom.c | 17 ++++++++++++-----
> >>> 1 file changed, 12 insertions(+), 5 deletions(-)
> >>>
> >>> diff --git a/drivers/soc/qcom/llcc-qcom.c b/drivers/soc/qcom/llcc-qcom.c
> >>> index 7b7c5a38bac6..8d840702df50 100644
> >>> --- a/drivers/soc/qcom/llcc-qcom.c
> >>> +++ b/drivers/soc/qcom/llcc-qcom.c
> >>> @@ -1012,11 +1012,18 @@ static int qcom_llcc_probe(struct platform_device *pdev)
> >>>
> >>> drv_data->ecc_irq = platform_get_irq_optional(pdev, 0);
> >>>
> >>> - llcc_edac = platform_device_register_data(&pdev->dev,
> >>> - "qcom_llcc_edac", -1, drv_data,
> >>> - sizeof(*drv_data));
> >>> - if (IS_ERR(llcc_edac))
> >>> - dev_err(dev, "Failed to register llcc edac driver\n");
> >>> + /*
> >>> + * The platforms based on SDM845 SoC locks the access to EDAC registers
> >>> + * in bootloader. So probing the EDAC driver will result in a crash.
> >>> + * Hence, disable the creation of EDAC platform device on SDM845.
> >>> + */
> >>> + if (!of_device_is_compatible(dev->of_node, "qcom,sdm845-llcc")) {
> >>
> >> Don't spread of_device_is_compatible() in driver code. You have driver
> >> data for this.
> >>
> >
> > Yeah, but there is no ID to in the driver data to identify an SoC.
>
> What do you mean there is no? You use exactly the same compatible as the
> one in driver data.
>
Right, but I was saying that there is no unique field to identify an SoC.
>
> > I could add
> > one but is that really worth doing so? Is using of_device_is_compatible() in
> > drivers discouraged nowadays?
>
> Because it spreads variant matching all over. It does not scale. drv
> data fields are the way or better quirks/flags.
>
The driver quirk/flags are usually beneficial if it applies to multiple
platforms, otherwise they are a bit overkill IMO just like in this case.
One can argue that this matching could spread to other SoCs in the future, but
I don't think that could happen for this case.
Thanks,
Mani
> Best regards,
> Krzysztof
>
--
மணிவண்ணன் சதாசிவம்
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v6 17/17] soc: qcom: llcc: Do not create EDAC platform device on SDM845
2023-01-18 16:26 ` Manivannan Sadhasivam
@ 2023-01-18 17:06 ` Krzysztof Kozlowski
0 siblings, 0 replies; 12+ messages in thread
From: Krzysztof Kozlowski @ 2023-01-18 17:06 UTC (permalink / raw)
To: Manivannan Sadhasivam
Cc: andersson, robh+dt, krzysztof.kozlowski+dt, bp, tony.luck,
quic_saipraka, konrad.dybcio, linux-arm-msm, linux-kernel,
james.morse, mchehab, rric, linux-edac, quic_ppareek, luca.weiss,
ahalaney, steev, stable
On 18/01/2023 17:26, Manivannan Sadhasivam wrote:
> On Wed, Jan 18, 2023 at 05:05:28PM +0100, Krzysztof Kozlowski wrote:
>> On 18/01/2023 16:59, Manivannan Sadhasivam wrote:
>>> On Wed, Jan 18, 2023 at 04:37:29PM +0100, Krzysztof Kozlowski wrote:
>>>> On 18/01/2023 16:09, Manivannan Sadhasivam wrote:
>>>>> The platforms based on SDM845 SoC locks the access to EDAC registers in the
>>>>> bootloader. So probing the EDAC driver will result in a crash. Hence,
>>>>> disable the creation of EDAC platform device on all SDM845 devices.
>>>>>
>>>>> The issue has been observed on Lenovo Yoga C630 and DB845c.
>>>>>
>>>>> Cc: <stable@vger.kernel.org> # 5.10
>>>>> Reported-by: Steev Klimaszewski <steev@kali.org>
>>>>> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
>>>>> ---
>>>>> drivers/soc/qcom/llcc-qcom.c | 17 ++++++++++++-----
>>>>> 1 file changed, 12 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/drivers/soc/qcom/llcc-qcom.c b/drivers/soc/qcom/llcc-qcom.c
>>>>> index 7b7c5a38bac6..8d840702df50 100644
>>>>> --- a/drivers/soc/qcom/llcc-qcom.c
>>>>> +++ b/drivers/soc/qcom/llcc-qcom.c
>>>>> @@ -1012,11 +1012,18 @@ static int qcom_llcc_probe(struct platform_device *pdev)
>>>>>
>>>>> drv_data->ecc_irq = platform_get_irq_optional(pdev, 0);
>>>>>
>>>>> - llcc_edac = platform_device_register_data(&pdev->dev,
>>>>> - "qcom_llcc_edac", -1, drv_data,
>>>>> - sizeof(*drv_data));
>>>>> - if (IS_ERR(llcc_edac))
>>>>> - dev_err(dev, "Failed to register llcc edac driver\n");
>>>>> + /*
>>>>> + * The platforms based on SDM845 SoC locks the access to EDAC registers
>>>>> + * in bootloader. So probing the EDAC driver will result in a crash.
>>>>> + * Hence, disable the creation of EDAC platform device on SDM845.
>>>>> + */
>>>>> + if (!of_device_is_compatible(dev->of_node, "qcom,sdm845-llcc")) {
>>>>
>>>> Don't spread of_device_is_compatible() in driver code. You have driver
>>>> data for this.
>>>>
>>>
>>> Yeah, but there is no ID to in the driver data to identify an SoC.
>>
>> What do you mean there is no? You use exactly the same compatible as the
>> one in driver data.
>>
>
> Right, but I was saying that there is no unique field to identify an SoC.
>
>>
>>> I could add
>>> one but is that really worth doing so? Is using of_device_is_compatible() in
>>> drivers discouraged nowadays?
>>
>> Because it spreads variant matching all over. It does not scale. drv
>> data fields are the way or better quirks/flags.
>>
>
> The driver quirk/flags are usually beneficial if it applies to multiple
> platforms, otherwise they are a bit overkill IMO just like in this case.
>
> One can argue that this matching could spread to other SoCs in the future, but
> I don't think that could happen for this case.
That's the argument for every flag/quirk/field. Driver already uses it -
see need_llcc_cfg being set for only one (!!!) variant. Now you add
orthogonal field just as of_device_is_compatible(). No, that's why we
have driver data and as I said - it is already used.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v6 01/17] EDAC/device: Respect any driver-supplied workqueue polling value
2023-01-18 15:08 ` [PATCH v6 01/17] EDAC/device: Respect any driver-supplied workqueue polling value Manivannan Sadhasivam
@ 2023-01-18 17:46 ` Bjorn Andersson
2023-01-18 19:05 ` Borislav Petkov
2023-01-19 10:55 ` Borislav Petkov
1 sibling, 1 reply; 12+ messages in thread
From: Bjorn Andersson @ 2023-01-18 17:46 UTC (permalink / raw)
To: Manivannan Sadhasivam
Cc: robh+dt, krzysztof.kozlowski+dt, bp, tony.luck, quic_saipraka,
konrad.dybcio, linux-arm-msm, linux-kernel, james.morse, mchehab,
rric, linux-edac, quic_ppareek, luca.weiss, ahalaney, steev,
stable
On Wed, Jan 18, 2023 at 08:38:48PM +0530, Manivannan Sadhasivam wrote:
> The EDAC drivers may optionally pass the poll_msec value. Use that value
> if available, else fall back to 1000ms.
>
> [ bp: Touchups. ]
>
> Fixes: e27e3dac6517 ("drivers/edac: add edac_device class")
> Reported-by: Luca Weiss <luca.weiss@fairphone.com>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Your S-o-b should be the last one to indicate that you are the one
certifying the origin of this patch.
> Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de>
If the two of you wrote the patch, please add a Co-developed-by.
Thanks,
Bjorn
> Tested-by: Steev Klimaszewski <steev@kali.org> # Thinkpad X13s
> Tested-by: Andrew Halaney <ahalaney@redhat.com> # sa8540p-ride
> Cc: <stable@vger.kernel.org> # 4.9
> Link: https://lore.kernel.org/r/COZYL8MWN97H.MROQ391BGA09@otso
> ---
> drivers/edac/edac_device.c | 15 +++++++--------
> 1 file changed, 7 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/edac/edac_device.c b/drivers/edac/edac_device.c
> index 19522c568aa5..a50b7bcfb731 100644
> --- a/drivers/edac/edac_device.c
> +++ b/drivers/edac/edac_device.c
> @@ -34,6 +34,9 @@
> static DEFINE_MUTEX(device_ctls_mutex);
> static LIST_HEAD(edac_device_list);
>
> +/* Default workqueue processing interval on this instance, in msecs */
> +#define DEFAULT_POLL_INTERVAL 1000
> +
> #ifdef CONFIG_EDAC_DEBUG
> static void edac_device_dump_device(struct edac_device_ctl_info *edac_dev)
> {
> @@ -336,7 +339,7 @@ static void edac_device_workq_function(struct work_struct *work_req)
> * whole one second to save timers firing all over the period
> * between integral seconds
> */
> - if (edac_dev->poll_msec == 1000)
> + if (edac_dev->poll_msec == DEFAULT_POLL_INTERVAL)
> edac_queue_work(&edac_dev->work, round_jiffies_relative(edac_dev->delay));
> else
> edac_queue_work(&edac_dev->work, edac_dev->delay);
> @@ -366,7 +369,7 @@ static void edac_device_workq_setup(struct edac_device_ctl_info *edac_dev,
> * timers firing on sub-second basis, while they are happy
> * to fire together on the 1 second exactly
> */
> - if (edac_dev->poll_msec == 1000)
> + if (edac_dev->poll_msec == DEFAULT_POLL_INTERVAL)
> edac_queue_work(&edac_dev->work, round_jiffies_relative(edac_dev->delay));
> else
> edac_queue_work(&edac_dev->work, edac_dev->delay);
> @@ -398,7 +401,7 @@ void edac_device_reset_delay_period(struct edac_device_ctl_info *edac_dev,
> {
> unsigned long jiffs = msecs_to_jiffies(value);
>
> - if (value == 1000)
> + if (value == DEFAULT_POLL_INTERVAL)
> jiffs = round_jiffies_relative(value);
>
> edac_dev->poll_msec = value;
> @@ -443,11 +446,7 @@ int edac_device_add_device(struct edac_device_ctl_info *edac_dev)
> /* This instance is NOW RUNNING */
> edac_dev->op_state = OP_RUNNING_POLL;
>
> - /*
> - * enable workq processing on this instance,
> - * default = 1000 msec
> - */
> - edac_device_workq_setup(edac_dev, 1000);
> + edac_device_workq_setup(edac_dev, edac_dev->poll_msec ?: DEFAULT_POLL_INTERVAL);
> } else {
> edac_dev->op_state = OP_RUNNING_INTERRUPT;
> }
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v6 01/17] EDAC/device: Respect any driver-supplied workqueue polling value
2023-01-18 17:46 ` Bjorn Andersson
@ 2023-01-18 19:05 ` Borislav Petkov
0 siblings, 0 replies; 12+ messages in thread
From: Borislav Petkov @ 2023-01-18 19:05 UTC (permalink / raw)
To: Bjorn Andersson
Cc: Manivannan Sadhasivam, robh+dt, krzysztof.kozlowski+dt, tony.luck,
quic_saipraka, konrad.dybcio, linux-arm-msm, linux-kernel,
james.morse, mchehab, rric, linux-edac, quic_ppareek, luca.weiss,
ahalaney, steev, stable
On Wed, Jan 18, 2023 at 11:46:25AM -0600, Bjorn Andersson wrote:
> On Wed, Jan 18, 2023 at 08:38:48PM +0530, Manivannan Sadhasivam wrote:
> > The EDAC drivers may optionally pass the poll_msec value. Use that value
> > if available, else fall back to 1000ms.
> >
> > [ bp: Touchups. ]
> >
> > Fixes: e27e3dac6517 ("drivers/edac: add edac_device class")
> > Reported-by: Luca Weiss <luca.weiss@fairphone.com>
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
>
> Your S-o-b should be the last one to indicate that you are the one
> certifying the origin of this patch.
I took his and massaged it a bit.
I'll fix up the order properly when applying.
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v6 01/17] EDAC/device: Respect any driver-supplied workqueue polling value
2023-01-18 15:08 ` [PATCH v6 01/17] EDAC/device: Respect any driver-supplied workqueue polling value Manivannan Sadhasivam
2023-01-18 17:46 ` Bjorn Andersson
@ 2023-01-19 10:55 ` Borislav Petkov
1 sibling, 0 replies; 12+ messages in thread
From: Borislav Petkov @ 2023-01-19 10:55 UTC (permalink / raw)
To: Manivannan Sadhasivam
Cc: andersson, robh+dt, krzysztof.kozlowski+dt, tony.luck,
quic_saipraka, konrad.dybcio, linux-arm-msm, linux-kernel,
james.morse, mchehab, rric, linux-edac, quic_ppareek, luca.weiss,
ahalaney, steev, stable
On Wed, Jan 18, 2023 at 08:38:48PM +0530, Manivannan Sadhasivam wrote:
> The EDAC drivers may optionally pass the poll_msec value. Use that value
> if available, else fall back to 1000ms.
>
> [ bp: Touchups. ]
>
> Fixes: e27e3dac6517 ("drivers/edac: add edac_device class")
> Reported-by: Luca Weiss <luca.weiss@fairphone.com>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de>
> Tested-by: Steev Klimaszewski <steev@kali.org> # Thinkpad X13s
> Tested-by: Andrew Halaney <ahalaney@redhat.com> # sa8540p-ride
> Cc: <stable@vger.kernel.org> # 4.9
> Link: https://lore.kernel.org/r/COZYL8MWN97H.MROQ391BGA09@otso
> ---
> drivers/edac/edac_device.c | 15 +++++++--------
> 1 file changed, 7 insertions(+), 8 deletions(-)
Applied, thanks.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v6 03/17] EDAC/qcom: Do not pass llcc_driv_data as edac_device_ctl_info's pvt_info
2023-01-18 15:08 ` [PATCH v6 03/17] EDAC/qcom: Do not pass llcc_driv_data as edac_device_ctl_info's pvt_info Manivannan Sadhasivam
@ 2023-01-20 18:56 ` Borislav Petkov
0 siblings, 0 replies; 12+ messages in thread
From: Borislav Petkov @ 2023-01-20 18:56 UTC (permalink / raw)
To: Manivannan Sadhasivam
Cc: andersson, robh+dt, krzysztof.kozlowski+dt, tony.luck,
quic_saipraka, konrad.dybcio, linux-arm-msm, linux-kernel,
james.morse, mchehab, rric, linux-edac, quic_ppareek, luca.weiss,
ahalaney, steev, stable
On Wed, Jan 18, 2023 at 08:38:50PM +0530, Manivannan Sadhasivam wrote:
> The memory for "llcc_driv_data" is allocated by the LLCC driver. But when
> it is passed as "pvt_info" to the EDAC core, it will get freed during the
> qcom_edac driver release. So when the qcom_edac driver gets probed again,
> it will try to use the freed data leading to the use-after-free bug.
>
> Hence, do not pass "llcc_driv_data" as pvt_info but rather reference it
> using the "platform_data" in the qcom_edac driver.
>
> Cc: <stable@vger.kernel.org> # 4.20
> Fixes: 27450653f1db ("drivers: edac: Add EDAC driver support for QCOM SoCs")
> Tested-by: Steev Klimaszewski <steev@kali.org> # Thinkpad X13s
> Tested-by: Andrew Halaney <ahalaney@redhat.com> # sa8540p-ride
> Reported-by: Steev Klimaszewski <steev@kali.org>
> Reviewed-by: Borislav Petkov (AMD) <bp@alien8.de>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
> drivers/edac/qcom_edac.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
Applied, thanks.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2023-01-20 18:56 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20230118150904.26913-1-manivannan.sadhasivam@linaro.org>
2023-01-18 15:08 ` [PATCH v6 01/17] EDAC/device: Respect any driver-supplied workqueue polling value Manivannan Sadhasivam
2023-01-18 17:46 ` Bjorn Andersson
2023-01-18 19:05 ` Borislav Petkov
2023-01-19 10:55 ` Borislav Petkov
2023-01-18 15:08 ` [PATCH v6 03/17] EDAC/qcom: Do not pass llcc_driv_data as edac_device_ctl_info's pvt_info Manivannan Sadhasivam
2023-01-20 18:56 ` Borislav Petkov
2023-01-18 15:09 ` [PATCH v6 17/17] soc: qcom: llcc: Do not create EDAC platform device on SDM845 Manivannan Sadhasivam
2023-01-18 15:37 ` Krzysztof Kozlowski
2023-01-18 15:59 ` Manivannan Sadhasivam
2023-01-18 16:05 ` Krzysztof Kozlowski
2023-01-18 16:26 ` Manivannan Sadhasivam
2023-01-18 17:06 ` Krzysztof Kozlowski
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox