[PATCH v6 01/17] EDAC/device: Respect any driver-supplied workqueue polling value

[PATCH v6 01/17] EDAC/device: Respect any driver-supplied workqueue polling value

[Manivannan Sadhasivam]

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

Re: [PATCH v6 01/17] EDAC/device: Respect any driver-supplied workqueue polling value

[Bjorn Andersson]

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
> 

Re: [PATCH v6 01/17] EDAC/device: Respect any driver-supplied workqueue polling value

[Borislav Petkov]

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

Re: [PATCH v6 01/17] EDAC/device: Respect any driver-supplied workqueue polling value

[Borislav Petkov]

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

[PATCH v6 03/17] EDAC/qcom: Do not pass llcc_driv_data as edac_device_ctl_info's pvt_info

[Manivannan Sadhasivam]

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

Re: [PATCH v6 03/17] EDAC/qcom: Do not pass llcc_driv_data as edac_device_ctl_info's pvt_info

[Borislav Petkov]

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

[PATCH v6 17/17] soc: qcom: llcc: Do not create EDAC platform device on SDM845

[Manivannan Sadhasivam]

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

Re: [PATCH v6 17/17] soc: qcom: llcc: Do not create EDAC platform device on SDM845

[Krzysztof Kozlowski]

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

Re: [PATCH v6 17/17] soc: qcom: llcc: Do not create EDAC platform device on SDM845

[Manivannan Sadhasivam]

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
> 

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v6 17/17] soc: qcom: llcc: Do not create EDAC platform device on SDM845

[Krzysztof Kozlowski]

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

Re: [PATCH v6 17/17] soc: qcom: llcc: Do not create EDAC platform device on SDM845

[Manivannan Sadhasivam]

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
> 

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v6 17/17] soc: qcom: llcc: Do not create EDAC platform device on SDM845

[Krzysztof Kozlowski]

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