| Message ID | 20240220122805.9084-1-quic_mojha@quicinc.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | soc: qcom: llcc: Add llcc device availability check | expand |
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;
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; >
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
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
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
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 >
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
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;
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(-)