[PATCH] soc: qcom: llcc: Add llcc device availability check

[PATCH] soc: qcom: llcc: Add llcc device availability check

[Mukesh Ojha]

When llcc driver is enabled  and llcc device is not
physically there on the SoC, client can get
-EPROBE_DEFER on calling llcc_slice_getd() and it
is possible they defer forever.

Let's add a check device availabilty and set the
appropriate applicable error in drv_data.

Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
---
 drivers/soc/qcom/llcc-qcom.c | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/drivers/soc/qcom/llcc-qcom.c b/drivers/soc/qcom/llcc-qcom.c
index 4ca88eaebf06..cb336b183bba 100644
--- a/drivers/soc/qcom/llcc-qcom.c
+++ b/drivers/soc/qcom/llcc-qcom.c
@@ -769,6 +769,27 @@ static const struct qcom_sct_config x1e80100_cfgs = {
 };
 
 static struct llcc_drv_data *drv_data = (void *) -EPROBE_DEFER;
+static DEFINE_MUTEX(dev_avail);
+
+static bool is_llcc_device_available(void)
+{
+	static struct llcc_drv_data *ptr;
+
+	mutex_lock(&dev_avail);
+	if (!ptr) {
+		struct device_node *node;
+
+		node = of_find_node_by_name(NULL, "system-cache-controller");
+		if (!of_device_is_available(node)) {
+			pr_warn("llcc-qcom: system-cache-controller node not found\n");
+			drv_data = ERR_PTR(-ENODEV);
+		}
+		of_node_put(node);
+		ptr = drv_data;
+	}
+	mutex_unlock(&dev_avail);
+	return (PTR_ERR(ptr) != -ENODEV) ? true : false;
+}
 
 /**
  * llcc_slice_getd - get llcc slice descriptor
@@ -783,7 +804,7 @@ struct llcc_slice_desc *llcc_slice_getd(u32 uid)
 	struct llcc_slice_desc *desc;
 	u32 sz, count;
 
-	if (IS_ERR(drv_data))
+	if (!is_llcc_device_available() || IS_ERR(drv_data))
 		return ERR_CAST(drv_data);
 
 	cfg = drv_data->cfg;
-- 
2.43.0.254.ga26002b62827

Re: [PATCH] soc: qcom: llcc: Add llcc device availability check

[Sahil Chandna]

On 2/20/2024 5:58 PM, Mukesh Ojha wrote:
> When llcc driver is enabled  and llcc device is not
> physically there on the SoC, client can get
> -EPROBE_DEFER on calling llcc_slice_getd() and it
> is possible they defer forever.
> 
> Let's add a check device availabilty and set the
> appropriate applicable error in drv_data.
> 
> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
> ---
>   drivers/soc/qcom/llcc-qcom.c | 23 ++++++++++++++++++++++-
>   1 file changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/soc/qcom/llcc-qcom.c b/drivers/soc/qcom/llcc-qcom.c
> index 4ca88eaebf06..cb336b183bba 100644
> --- a/drivers/soc/qcom/llcc-qcom.c
> +++ b/drivers/soc/qcom/llcc-qcom.c
> @@ -769,6 +769,27 @@ static const struct qcom_sct_config x1e80100_cfgs = {
>   };
>   
>   static struct llcc_drv_data *drv_data = (void *) -EPROBE_DEFER;
> +static DEFINE_MUTEX(dev_avail);
what is the requirement for mutex lock here? Since we are only trying to 
find if node present or not
> +
> +static bool is_llcc_device_available(void)
> +{
> +	static struct llcc_drv_data *ptr;
> +
> +	mutex_lock(&dev_avail);
> +	if (!ptr) {
> +		struct device_node *node;
> +
> +		node = of_find_node_by_name(NULL, "system-cache-controller");
> +		if (!of_device_is_available(node)) {
> +			pr_warn("llcc-qcom: system-cache-controller node not found\n");
> +			drv_data = ERR_PTR(-ENODEV);
> +		}
> +		of_node_put(node);
> +		ptr = drv_data;
> +	}
> +	mutex_unlock(&dev_avail);
> +	return (PTR_ERR(ptr) != -ENODEV) ? true : false;
> +}
>   
>   /**
>    * llcc_slice_getd - get llcc slice descriptor
> @@ -783,7 +804,7 @@ struct llcc_slice_desc *llcc_slice_getd(u32 uid)
>   	struct llcc_slice_desc *desc;
>   	u32 sz, count;
>   
> -	if (IS_ERR(drv_data))
> +	if (!is_llcc_device_available() || IS_ERR(drv_data))
>   		return ERR_CAST(drv_data);
>   
>   	cfg = drv_data->cfg;

Re: [PATCH] soc: qcom: llcc: Add llcc device availability check

[Mukesh Ojha]

On 2/22/2024 11:37 PM, Sahil Chandna wrote:
> On 2/20/2024 5:58 PM, Mukesh Ojha wrote:
>> When llcc driver is enabled  and llcc device is not
>> physically there on the SoC, client can get
>> -EPROBE_DEFER on calling llcc_slice_getd() and it
>> is possible they defer forever.
>>
>> Let's add a check device availabilty and set the
>> appropriate applicable error in drv_data.
>>
>> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
>> ---
>>   drivers/soc/qcom/llcc-qcom.c | 23 ++++++++++++++++++++++-
>>   1 file changed, 22 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/soc/qcom/llcc-qcom.c b/drivers/soc/qcom/llcc-qcom.c
>> index 4ca88eaebf06..cb336b183bba 100644
>> --- a/drivers/soc/qcom/llcc-qcom.c
>> +++ b/drivers/soc/qcom/llcc-qcom.c
>> @@ -769,6 +769,27 @@ static const struct qcom_sct_config x1e80100_cfgs 
>> = {
>>   };
>>   static struct llcc_drv_data *drv_data = (void *) -EPROBE_DEFER;
>> +static DEFINE_MUTEX(dev_avail);
> what is the requirement for mutex lock here? Since we are only trying to 
> find if node present or not

I was trying to avoid two parallel call from llcc_slice_getd() calling
parallel call to of_find_node_by_name() as it should be one time search 
for device presence to find a node and check if device is present or
not.

-Mukesh

>> +
>> +static bool is_llcc_device_available(void)
>> +{
>> +    static struct llcc_drv_data *ptr;
>> +
>> +    mutex_lock(&dev_avail);
>> +    if (!ptr) {
>> +        struct device_node *node;
>> +
>> +        node = of_find_node_by_name(NULL, "system-cache-controller");
>> +        if (!of_device_is_available(node)) {
>> +            pr_warn("llcc-qcom: system-cache-controller node not 
>> found\n");
>> +            drv_data = ERR_PTR(-ENODEV);
>> +        }
>> +        of_node_put(node);
>> +        ptr = drv_data;
>> +    }
>> +    mutex_unlock(&dev_avail);
>> +    return (PTR_ERR(ptr) != -ENODEV) ? true : false;
>> +}
>>   /**
>>    * llcc_slice_getd - get llcc slice descriptor
>> @@ -783,7 +804,7 @@ struct llcc_slice_desc *llcc_slice_getd(u32 uid)
>>       struct llcc_slice_desc *desc;
>>       u32 sz, count;
>> -    if (IS_ERR(drv_data))
>> +    if (!is_llcc_device_available() || IS_ERR(drv_data))
>>           return ERR_CAST(drv_data);
>>       cfg = drv_data->cfg;
> 

Re: [PATCH] soc: qcom: llcc: Add llcc device availability check

[Sahil Chandna]

On 2/26/2024 4:02 PM, Mukesh Ojha wrote:
> 
> 
> On 2/22/2024 11:37 PM, Sahil Chandna wrote:
>> On 2/20/2024 5:58 PM, Mukesh Ojha wrote:
>>> When llcc driver is enabled  and llcc device is not
>>> physically there on the SoC, client can get
>>> -EPROBE_DEFER on calling llcc_slice_getd() and it
>>> is possible they defer forever.
>>>
>>> Let's add a check device availabilty and set the
>>> appropriate applicable error in drv_data.
>>>
>>> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
>>> ---
>>>   drivers/soc/qcom/llcc-qcom.c | 23 ++++++++++++++++++++++-
>>>   1 file changed, 22 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/soc/qcom/llcc-qcom.c b/drivers/soc/qcom/llcc-qcom.c
>>> index 4ca88eaebf06..cb336b183bba 100644
>>> --- a/drivers/soc/qcom/llcc-qcom.c
>>> +++ b/drivers/soc/qcom/llcc-qcom.c
>>> @@ -769,6 +769,27 @@ static const struct qcom_sct_config 
>>> x1e80100_cfgs = {
>>>   };
>>>   static struct llcc_drv_data *drv_data = (void *) -EPROBE_DEFER;
>>> +static DEFINE_MUTEX(dev_avail);
>> what is the requirement for mutex lock here? Since we are only trying 
>> to find if node present or not
> 
> I was trying to avoid two parallel call from llcc_slice_getd() calling
> parallel call to of_find_node_by_name() as it should be one time search 
> for device presence to find a node and check if device is present or
> not.
> 
> -Mukesh
> 
Got it, but of_find_node_by_name () is holding a raw_spin_lock_irqsave 
() for concurrency, right ? please correct me if understanding is wrong.
>>> +
>>> +static bool is_llcc_device_available(void)
>>> +{
>>> +    static struct llcc_drv_data *ptr;
>>> +
>>> +    mutex_lock(&dev_avail);
>>> +    if (!ptr) {
>>> +        struct device_node *node;
>>> +
>>> +        node = of_find_node_by_name(NULL, "system-cache-controller");
>>> +        if (!of_device_is_available(node)) {
>>> +            pr_warn("llcc-qcom: system-cache-controller node not 
>>> found\n");
>>> +            drv_data = ERR_PTR(-ENODEV);
>>> +        }
>>> +        of_node_put(node);
>>> +        ptr = drv_data;
>>> +    }
>>> +    mutex_unlock(&dev_avail);
>>> +    return (PTR_ERR(ptr) != -ENODEV) ? true : false;
>>> +}
>>>   /**
>>>    * llcc_slice_getd - get llcc slice descriptor
>>> @@ -783,7 +804,7 @@ struct llcc_slice_desc *llcc_slice_getd(u32 uid)
>>>       struct llcc_slice_desc *desc;
>>>       u32 sz, count;
>>> -    if (IS_ERR(drv_data))
>>> +    if (!is_llcc_device_available() || IS_ERR(drv_data))
Also, thinking about this, should the status of device present or not be 
saved in static variable instead of function call for each client ?
>>>           return ERR_CAST(drv_data);
>>>       cfg = drv_data->cfg;
>>

Regards,
Sahil

Re: [PATCH] soc: qcom: llcc: Add llcc device availability check

[Mukesh Ojha]

On 2/26/2024 4:19 PM, Sahil Chandna wrote:
> On 2/26/2024 4:02 PM, Mukesh Ojha wrote:
>>
>>
>> On 2/22/2024 11:37 PM, Sahil Chandna wrote:
>>> On 2/20/2024 5:58 PM, Mukesh Ojha wrote:
>>>> When llcc driver is enabled  and llcc device is not
>>>> physically there on the SoC, client can get
>>>> -EPROBE_DEFER on calling llcc_slice_getd() and it
>>>> is possible they defer forever.
>>>>
>>>> Let's add a check device availabilty and set the
>>>> appropriate applicable error in drv_data.
>>>>
>>>> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
>>>> ---
>>>>   drivers/soc/qcom/llcc-qcom.c | 23 ++++++++++++++++++++++-
>>>>   1 file changed, 22 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/soc/qcom/llcc-qcom.c 
>>>> b/drivers/soc/qcom/llcc-qcom.c
>>>> index 4ca88eaebf06..cb336b183bba 100644
>>>> --- a/drivers/soc/qcom/llcc-qcom.c
>>>> +++ b/drivers/soc/qcom/llcc-qcom.c
>>>> @@ -769,6 +769,27 @@ static const struct qcom_sct_config 
>>>> x1e80100_cfgs = {
>>>>   };
>>>>   static struct llcc_drv_data *drv_data = (void *) -EPROBE_DEFER;
>>>> +static DEFINE_MUTEX(dev_avail);
>>> what is the requirement for mutex lock here? Since we are only trying 
>>> to find if node present or not
>>
>> I was trying to avoid two parallel call from llcc_slice_getd() calling
>> parallel call to of_find_node_by_name() as it should be one time 
>> search for device presence to find a node and check if device is 
>> present or
>> not.
>>
>> -Mukesh
>>
> Got it, but of_find_node_by_name () is holding a raw_spin_lock_irqsave 
> () for concurrency, right ? please correct me if understanding is wrong.

Even though, of_find_node_by_name () is holding spin_lock but nothing
is preventing the 2nd call from happening. Here, mutex and check on
!ptr ensures, we don't make 2nd call.

-Mukesh
>>>> +
>>>> +static bool is_llcc_device_available(void)
>>>> +{
>>>> +    static struct llcc_drv_data *ptr;
>>>> +
>>>> +    mutex_lock(&dev_avail);
>>>> +    if (!ptr) {
>>>> +        struct device_node *node;
>>>> +
>>>> +        node = of_find_node_by_name(NULL, "system-cache-controller");
>>>> +        if (!of_device_is_available(node)) {
>>>> +            pr_warn("llcc-qcom: system-cache-controller node not 
>>>> found\n");
>>>> +            drv_data = ERR_PTR(-ENODEV);
>>>> +        }
>>>> +        of_node_put(node);
>>>> +        ptr = drv_data;
>>>> +    }
>>>> +    mutex_unlock(&dev_avail);
>>>> +    return (PTR_ERR(ptr) != -ENODEV) ? true : false;
>>>> +}
>>>>   /**
>>>>    * llcc_slice_getd - get llcc slice descriptor
>>>> @@ -783,7 +804,7 @@ struct llcc_slice_desc *llcc_slice_getd(u32 uid)
>>>>       struct llcc_slice_desc *desc;
>>>>       u32 sz, count;
>>>> -    if (IS_ERR(drv_data))
>>>> +    if (!is_llcc_device_available() || IS_ERR(drv_data))
> Also, thinking about this, should the status of device present or not be 
> saved in static variable instead of function call for each client ?
>>>>           return ERR_CAST(drv_data);
>>>>       cfg = drv_data->cfg;
>>>
> 
> Regards,
> Sahil

Re: [PATCH] soc: qcom: llcc: Add llcc device availability check

[Krzysztof Kozlowski]

On 20/02/2024 13:28, Mukesh Ojha wrote:
> When llcc driver is enabled  and llcc device is not
> physically there on the SoC, client can get
> -EPROBE_DEFER on calling llcc_slice_getd() and it
> is possible they defer forever.

Please wrap commit message according to Linux coding style / submission
process (neither too early nor over the limit):
https://elixir.bootlin.com/linux/v6.4-rc1/source/Documentation/process/submitting-patches.rst#L597

> 
> Let's add a check device availabilty and set the
> appropriate applicable error in drv_data.
> 
> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
> ---
>  drivers/soc/qcom/llcc-qcom.c | 23 ++++++++++++++++++++++-
>  1 file changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/soc/qcom/llcc-qcom.c b/drivers/soc/qcom/llcc-qcom.c
> index 4ca88eaebf06..cb336b183bba 100644
> --- a/drivers/soc/qcom/llcc-qcom.c
> +++ b/drivers/soc/qcom/llcc-qcom.c
> @@ -769,6 +769,27 @@ static const struct qcom_sct_config x1e80100_cfgs = {
>  };
>  
>  static struct llcc_drv_data *drv_data = (void *) -EPROBE_DEFER;
> +static DEFINE_MUTEX(dev_avail);
> +
> +static bool is_llcc_device_available(void)
> +{
> +	static struct llcc_drv_data *ptr;
> +
> +	mutex_lock(&dev_avail);
> +	if (!ptr) {
> +		struct device_node *node;
> +
> +		node = of_find_node_by_name(NULL, "system-cache-controller");

Why do you look names by name? This create undocumented ABI.

NAK (also for any future uses of such of_find_node_by_name()).

Best regards,
Krzysztof

Re: [PATCH] soc: qcom: llcc: Add llcc device availability check

[Mukesh Ojha]

On 3/7/2024 3:51 PM, Krzysztof Kozlowski wrote:
> On 20/02/2024 13:28, Mukesh Ojha wrote:
>> When llcc driver is enabled  and llcc device is not
>> physically there on the SoC, client can get
>> -EPROBE_DEFER on calling llcc_slice_getd() and it
>> is possible they defer forever.
> 
> Please wrap commit message according to Linux coding style / submission
> process (neither too early nor over the limit):
> https://elixir.bootlin.com/linux/v6.4-rc1/source/Documentation/process/submitting-patches.rst#L597

Noted.

> 
>>
>> Let's add a check device availabilty and set the
>> appropriate applicable error in drv_data.
>>
>> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
>> ---
>>   drivers/soc/qcom/llcc-qcom.c | 23 ++++++++++++++++++++++-
>>   1 file changed, 22 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/soc/qcom/llcc-qcom.c b/drivers/soc/qcom/llcc-qcom.c
>> index 4ca88eaebf06..cb336b183bba 100644
>> --- a/drivers/soc/qcom/llcc-qcom.c
>> +++ b/drivers/soc/qcom/llcc-qcom.c
>> @@ -769,6 +769,27 @@ static const struct qcom_sct_config x1e80100_cfgs = {
>>   };
>>   
>>   static struct llcc_drv_data *drv_data = (void *) -EPROBE_DEFER;
>> +static DEFINE_MUTEX(dev_avail);
>> +
>> +static bool is_llcc_device_available(void)
>> +{
>> +	static struct llcc_drv_data *ptr;
>> +
>> +	mutex_lock(&dev_avail);
>> +	if (!ptr) {
>> +		struct device_node *node;
>> +
>> +		node = of_find_node_by_name(NULL, "system-cache-controller");
> 
> Why do you look names by name? This create undocumented ABI. >
> NAK (also for any future uses of such of_find_node_by_name()).

I agree, what if we add a common compatible string like qcom,llcc to all 
llcc supported SoCs.

-Mukesh
> 
> Best regards,
> Krzysztof
> 

Re: [PATCH] soc: qcom: llcc: Add llcc device availability check

[Krzysztof Kozlowski]

On 12/03/2024 17:25, Mukesh Ojha wrote:
>>>   static struct llcc_drv_data *drv_data = (void *) -EPROBE_DEFER;
>>> +static DEFINE_MUTEX(dev_avail);
>>> +
>>> +static bool is_llcc_device_available(void)
>>> +{
>>> +	static struct llcc_drv_data *ptr;
>>> +
>>> +	mutex_lock(&dev_avail);
>>> +	if (!ptr) {
>>> +		struct device_node *node;
>>> +
>>> +		node = of_find_node_by_name(NULL, "system-cache-controller");
>>
>> Why do you look names by name? This create undocumented ABI. >
>> NAK (also for any future uses of such of_find_node_by_name()).
> 
> I agree, what if we add a common compatible string like qcom,llcc to all 
> llcc supported SoCs.

I did not dig into the your problem (also commit msg does not really
help me in that), but usually relationship between device nodes is
expressed with phandles.

This also has benefits of easier (future) integration with device links
and probe ordering.

Best regards,
Krzysztof