diff mbox series

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

Message ID 20230118150904.26913-18-manivannan.sadhasivam@linaro.org
State New
Headers
Series Qcom: LLCC/EDAC: Fix base address used for LLCC banks |

Commit Message

Manivannan Sadhasivam Jan. 18, 2023, 3:09 p.m. UTC 
  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(-)

Comments

Krzysztof Kozlowski Jan. 18, 2023, 3:37 p.m. UTC | #1 
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
Manivannan Sadhasivam Jan. 18, 2023, 3:59 p.m. UTC | #2 
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
>
Krzysztof Kozlowski Jan. 18, 2023, 4:05 p.m. UTC | #3 
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
Manivannan Sadhasivam Jan. 18, 2023, 4:26 p.m. UTC | #4 
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
>
Krzysztof Kozlowski Jan. 18, 2023, 5:06 p.m. UTC | #5 
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
diff mbox series

Patch

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: