Having greater than AHCI_MAX_PORTS (32) ports detected isn't that critical
from the further AHCI-platform initialization point of view since
exceeding the ports upper limit will cause allocating more resources than
will be used afterwards. But detecting too many child DT-nodes doesn't
seem right since it's very unlikely to have it on an ordinary platform. In
accordance with the AHCI specification there can't be more than 32 ports
implemented at least due to having the CAP.NP field of 5 bits wide and the
PI register of dword size. Thus if such situation is found the DTB must
have been corrupted and the data read from it shouldn't be reliable. Let's
consider that as an erroneous situation and halt further resources
allocation.
Note it's logically more correct to have the nports set only after the
initialization value is checked for being sane. So while at it let's make
sure nports is assigned with a correct value.
Signed-off-by: Serge Semin <[email protected]>
Reviewed-by: Hannes Reinecke <[email protected]>
---
Changelog v2:
- Drop the else word from the child_nodes value checking if-else-if
statement (@Damien) and convert the after-else part into the ternary
operator-based statement.
Changelog v4:
- Fix some logical mistakes in the patch log. (@Sergei Shtylyov)
---
drivers/ata/libahci_platform.c | 13 ++++++++++---
1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c
index 814804582d1d..8aed7b29c7ab 100644
--- a/drivers/ata/libahci_platform.c
+++ b/drivers/ata/libahci_platform.c
@@ -451,15 +451,22 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev,
}
}
- hpriv->nports = child_nodes = of_get_child_count(dev->of_node);
+ /*
+ * Too many sub-nodes most likely means having something wrong with
+ * the firmware.
+ */
+ child_nodes = of_get_child_count(dev->of_node);
+ if (child_nodes > AHCI_MAX_PORTS) {
+ rc = -EINVAL;
+ goto err_out;
+ }
/*
* If no sub-node was found, we still need to set nports to
* one in order to be able to use the
* ahci_platform_[en|dis]able_[phys|regulators] functions.
*/
- if (!child_nodes)
- hpriv->nports = 1;
+ hpriv->nports = child_nodes ?: 1;
hpriv->phys = devm_kcalloc(dev, hpriv->nports, sizeof(*hpriv->phys), GFP_KERNEL);
if (!hpriv->phys) {
--
2.35.1
On 6/10/22 17:17, Serge Semin wrote:
> Having greater than AHCI_MAX_PORTS (32) ports detected isn't that critical
> from the further AHCI-platform initialization point of view since
> exceeding the ports upper limit will cause allocating more resources than
> will be used afterwards. But detecting too many child DT-nodes doesn't
> seem right since it's very unlikely to have it on an ordinary platform. In
> accordance with the AHCI specification there can't be more than 32 ports
> implemented at least due to having the CAP.NP field of 5 bits wide and the
> PI register of dword size. Thus if such situation is found the DTB must
> have been corrupted and the data read from it shouldn't be reliable. Let's
> consider that as an erroneous situation and halt further resources
> allocation.
>
> Note it's logically more correct to have the nports set only after the
> initialization value is checked for being sane. So while at it let's make
> sure nports is assigned with a correct value.
>
> Signed-off-by: Serge Semin <[email protected]>
> Reviewed-by: Hannes Reinecke <[email protected]>
>
> ---
>
> Changelog v2:
> - Drop the else word from the child_nodes value checking if-else-if
> statement (@Damien) and convert the after-else part into the ternary
> operator-based statement.
>
> Changelog v4:
> - Fix some logical mistakes in the patch log. (@Sergei Shtylyov)
> ---
> drivers/ata/libahci_platform.c | 13 ++++++++++---
> 1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c
> index 814804582d1d..8aed7b29c7ab 100644
> --- a/drivers/ata/libahci_platform.c
> +++ b/drivers/ata/libahci_platform.c
> @@ -451,15 +451,22 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev,
> }
> }
>
> - hpriv->nports = child_nodes = of_get_child_count(dev->of_node);
> + /*
> + * Too many sub-nodes most likely means having something wrong with
> + * the firmware.
> + */
> + child_nodes = of_get_child_count(dev->of_node);
> + if (child_nodes > AHCI_MAX_PORTS) {
> + rc = -EINVAL;
> + goto err_out;
> + }
>
> /*
> * If no sub-node was found, we still need to set nports to
> * one in order to be able to use the
> * ahci_platform_[en|dis]able_[phys|regulators] functions.
> */
> - if (!child_nodes)
> - hpriv->nports = 1;
> + hpriv->nports = child_nodes ?: 1;
This change is not necessary and makes the code far less easy to read.
>
> hpriv->phys = devm_kcalloc(dev, hpriv->nports, sizeof(*hpriv->phys), GFP_KERNEL);
> if (!hpriv->phys) {
--
Damien Le Moal
Western Digital Research
On Tue, Jun 14, 2022 at 05:23:33PM +0900, Damien Le Moal wrote:
> On 6/10/22 17:17, Serge Semin wrote:
> > Having greater than AHCI_MAX_PORTS (32) ports detected isn't that critical
> > from the further AHCI-platform initialization point of view since
> > exceeding the ports upper limit will cause allocating more resources than
> > will be used afterwards. But detecting too many child DT-nodes doesn't
> > seem right since it's very unlikely to have it on an ordinary platform. In
> > accordance with the AHCI specification there can't be more than 32 ports
> > implemented at least due to having the CAP.NP field of 5 bits wide and the
> > PI register of dword size. Thus if such situation is found the DTB must
> > have been corrupted and the data read from it shouldn't be reliable. Let's
> > consider that as an erroneous situation and halt further resources
> > allocation.
> >
> > Note it's logically more correct to have the nports set only after the
> > initialization value is checked for being sane. So while at it let's make
> > sure nports is assigned with a correct value.
> >
> > Signed-off-by: Serge Semin <[email protected]>
> > Reviewed-by: Hannes Reinecke <[email protected]>
> >
> > ---
> >
> > Changelog v2:
> > - Drop the else word from the child_nodes value checking if-else-if
> > statement (@Damien) and convert the after-else part into the ternary
> > operator-based statement.
> >
> > Changelog v4:
> > - Fix some logical mistakes in the patch log. (@Sergei Shtylyov)
> > ---
> > drivers/ata/libahci_platform.c | 13 ++++++++++---
> > 1 file changed, 10 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c
> > index 814804582d1d..8aed7b29c7ab 100644
> > --- a/drivers/ata/libahci_platform.c
> > +++ b/drivers/ata/libahci_platform.c
> > @@ -451,15 +451,22 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev,
> > }
> > }
> >
> > - hpriv->nports = child_nodes = of_get_child_count(dev->of_node);
> > + /*
> > + * Too many sub-nodes most likely means having something wrong with
> > + * the firmware.
> > + */
> > + child_nodes = of_get_child_count(dev->of_node);
> > + if (child_nodes > AHCI_MAX_PORTS) {
> > + rc = -EINVAL;
> > + goto err_out;
> > + }
> >
> > /*
> > * If no sub-node was found, we still need to set nports to
> > * one in order to be able to use the
> > * ahci_platform_[en|dis]able_[phys|regulators] functions.
> > */
> > - if (!child_nodes)
> > - hpriv->nports = 1;
> > + hpriv->nports = child_nodes ?: 1;
>
> This change is not necessary and makes the code far less easy to read.
elaborate please. What change? What part of this change makes the code
less easy to read?
-Sergey
>
> >
> > hpriv->phys = devm_kcalloc(dev, hpriv->nports, sizeof(*hpriv->phys), GFP_KERNEL);
> > if (!hpriv->phys) {
>
>
> --
> Damien Le Moal
> Western Digital Research
On 2022/06/16 5:53, Serge Semin wrote:
> On Tue, Jun 14, 2022 at 05:23:33PM +0900, Damien Le Moal wrote:
>> On 6/10/22 17:17, Serge Semin wrote:
>>> Having greater than AHCI_MAX_PORTS (32) ports detected isn't that critical
>>> from the further AHCI-platform initialization point of view since
>>> exceeding the ports upper limit will cause allocating more resources than
>>> will be used afterwards. But detecting too many child DT-nodes doesn't
>>> seem right since it's very unlikely to have it on an ordinary platform. In
>>> accordance with the AHCI specification there can't be more than 32 ports
>>> implemented at least due to having the CAP.NP field of 5 bits wide and the
>>> PI register of dword size. Thus if such situation is found the DTB must
>>> have been corrupted and the data read from it shouldn't be reliable. Let's
>>> consider that as an erroneous situation and halt further resources
>>> allocation.
>>>
>>> Note it's logically more correct to have the nports set only after the
>>> initialization value is checked for being sane. So while at it let's make
>>> sure nports is assigned with a correct value.
>>>
>>> Signed-off-by: Serge Semin <[email protected]>
>>> Reviewed-by: Hannes Reinecke <[email protected]>
>>>
>>> ---
>>>
>>> Changelog v2:
>>> - Drop the else word from the child_nodes value checking if-else-if
>>> statement (@Damien) and convert the after-else part into the ternary
>>> operator-based statement.
>>>
>>> Changelog v4:
>>> - Fix some logical mistakes in the patch log. (@Sergei Shtylyov)
>>> ---
>>> drivers/ata/libahci_platform.c | 13 ++++++++++---
>>> 1 file changed, 10 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c
>>> index 814804582d1d..8aed7b29c7ab 100644
>>> --- a/drivers/ata/libahci_platform.c
>>> +++ b/drivers/ata/libahci_platform.c
>>> @@ -451,15 +451,22 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev,
>>> }
>>> }
>>>
>>> - hpriv->nports = child_nodes = of_get_child_count(dev->of_node);
>>> + /*
>>> + * Too many sub-nodes most likely means having something wrong with
>>> + * the firmware.
>>> + */
>>> + child_nodes = of_get_child_count(dev->of_node);
>>> + if (child_nodes > AHCI_MAX_PORTS) {
>>> + rc = -EINVAL;
>>> + goto err_out;
>>> + }
>>>
>>> /*
>>> * If no sub-node was found, we still need to set nports to
>>> * one in order to be able to use the
>>> * ahci_platform_[en|dis]able_[phys|regulators] functions.
>>> */
>>> - if (!child_nodes)
>>> - hpriv->nports = 1;
>>> + hpriv->nports = child_nodes ?: 1;
>>
>
>> This change is not necessary and makes the code far less easy to read.
>
> elaborate please. What change? What part of this change makes the code
> less easy to read?
You changed:
if (!child_nodes)
hpriv->nports = 1;
to:
hpriv->nports = child_nodes ?: 1;
That is the same. So the change is not needed in the first place, and worse,
makes the code way harder to read for no good reason.
>
> -Sergey
>
>>
>>>
>>> hpriv->phys = devm_kcalloc(dev, hpriv->nports, sizeof(*hpriv->phys), GFP_KERNEL);
>>> if (!hpriv->phys) {
>>
>>
>> --
>> Damien Le Moal
>> Western Digital Research
--
Damien Le Moal
Western Digital Research
On Thu, Jun 16, 2022 at 09:25:48AM +0900, Damien Le Moal wrote:
> On 2022/06/16 5:53, Serge Semin wrote:
> > On Tue, Jun 14, 2022 at 05:23:33PM +0900, Damien Le Moal wrote:
> >> On 6/10/22 17:17, Serge Semin wrote:
> >>> Having greater than AHCI_MAX_PORTS (32) ports detected isn't that critical
> >>> from the further AHCI-platform initialization point of view since
> >>> exceeding the ports upper limit will cause allocating more resources than
> >>> will be used afterwards. But detecting too many child DT-nodes doesn't
> >>> seem right since it's very unlikely to have it on an ordinary platform. In
> >>> accordance with the AHCI specification there can't be more than 32 ports
> >>> implemented at least due to having the CAP.NP field of 5 bits wide and the
> >>> PI register of dword size. Thus if such situation is found the DTB must
> >>> have been corrupted and the data read from it shouldn't be reliable. Let's
> >>> consider that as an erroneous situation and halt further resources
> >>> allocation.
> >>>
> >>> Note it's logically more correct to have the nports set only after the
> >>> initialization value is checked for being sane. So while at it let's make
> >>> sure nports is assigned with a correct value.
> >>>
> >>> Signed-off-by: Serge Semin <[email protected]>
> >>> Reviewed-by: Hannes Reinecke <[email protected]>
> >>>
> >>> ---
> >>>
> >>> Changelog v2:
> >>> - Drop the else word from the child_nodes value checking if-else-if
> >>> statement (@Damien) and convert the after-else part into the ternary
> >>> operator-based statement.
> >>>
> >>> Changelog v4:
> >>> - Fix some logical mistakes in the patch log. (@Sergei Shtylyov)
> >>> ---
> >>> drivers/ata/libahci_platform.c | 13 ++++++++++---
> >>> 1 file changed, 10 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c
> >>> index 814804582d1d..8aed7b29c7ab 100644
> >>> --- a/drivers/ata/libahci_platform.c
> >>> +++ b/drivers/ata/libahci_platform.c
> >>> @@ -451,15 +451,22 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev,
> >>> }
> >>> }
> >>>
> >>> - hpriv->nports = child_nodes = of_get_child_count(dev->of_node);
> >>> + /*
> >>> + * Too many sub-nodes most likely means having something wrong with
> >>> + * the firmware.
> >>> + */
> >>> + child_nodes = of_get_child_count(dev->of_node);
> >>> + if (child_nodes > AHCI_MAX_PORTS) {
> >>> + rc = -EINVAL;
> >>> + goto err_out;
> >>> + }
> >>>
> >>> /*
> >>> * If no sub-node was found, we still need to set nports to
> >>> * one in order to be able to use the
> >>> * ahci_platform_[en|dis]able_[phys|regulators] functions.
> >>> */
> >>> - if (!child_nodes)
> >>> - hpriv->nports = 1;
> >>> + hpriv->nports = child_nodes ?: 1;
> >>
> >
> >> This change is not necessary and makes the code far less easy to read.
> >
> > elaborate please. What change? What part of this change makes the code
> > less easy to read?
>
> You changed:
>
> if (!child_nodes)
> hpriv->nports = 1;
>
> to:
>
> hpriv->nports = child_nodes ?: 1;
>
> That is the same. So the change is not needed in the first place, and worse,
> makes the code way harder to read for no good reason.
No, they aren't the same:
+ if (!child_nodes)
+ hpriv->nports = 1;
and
+ hpriv->nports = child_nodes ?: 1;
aren't equivalent. The equivalent implementation would be:
+ if (child_nodes)
+ hpriv->nports = child_nodes;
+ else
+ hpriv->nports = 1;
As I said in the patchlog, hpriv->nports is updated now only if
of_get_child_count() returns a valid number of the child nodes,
ports, which semantically is more correct. In the previous
implementation it was always set to the number of child nodes
no matter whether that value was correct or not.
Regarding the ternary operator with omitted operand. Well, it's not
that rare beast in the kernel:
$ grep -r "?:" kernel/ drivers/ mm/ fs/ block/ | wc -l
699
But if you insist in it being not that readable, I can replace it with
more bulky if-else statement. Do you?
-Sergey
>
> >
> > -Sergey
> >
> >>
> >>>
> >>> hpriv->phys = devm_kcalloc(dev, hpriv->nports, sizeof(*hpriv->phys), GFP_KERNEL);
> >>> if (!hpriv->phys) {
> >>
> >>
> >> --
> >> Damien Le Moal
> >> Western Digital Research
>
>
> --
> Damien Le Moal
> Western Digital Research
On 6/18/22 05:18, Serge Semin wrote:
> On Thu, Jun 16, 2022 at 09:25:48AM +0900, Damien Le Moal wrote:
>> On 2022/06/16 5:53, Serge Semin wrote:
>>> On Tue, Jun 14, 2022 at 05:23:33PM +0900, Damien Le Moal wrote:
>>>> On 6/10/22 17:17, Serge Semin wrote:
>>>>> Having greater than AHCI_MAX_PORTS (32) ports detected isn't that critical
>>>>> from the further AHCI-platform initialization point of view since
>>>>> exceeding the ports upper limit will cause allocating more resources than
>>>>> will be used afterwards. But detecting too many child DT-nodes doesn't
>>>>> seem right since it's very unlikely to have it on an ordinary platform. In
>>>>> accordance with the AHCI specification there can't be more than 32 ports
>>>>> implemented at least due to having the CAP.NP field of 5 bits wide and the
>>>>> PI register of dword size. Thus if such situation is found the DTB must
>>>>> have been corrupted and the data read from it shouldn't be reliable. Let's
>>>>> consider that as an erroneous situation and halt further resources
>>>>> allocation.
>>>>>
>>>>> Note it's logically more correct to have the nports set only after the
>>>>> initialization value is checked for being sane. So while at it let's make
>>>>> sure nports is assigned with a correct value.
>>>>>
>>>>> Signed-off-by: Serge Semin <[email protected]>
>>>>> Reviewed-by: Hannes Reinecke <[email protected]>
>>>>>
>>>>> ---
>>>>>
>>>>> Changelog v2:
>>>>> - Drop the else word from the child_nodes value checking if-else-if
>>>>> statement (@Damien) and convert the after-else part into the ternary
>>>>> operator-based statement.
>>>>>
>>>>> Changelog v4:
>>>>> - Fix some logical mistakes in the patch log. (@Sergei Shtylyov)
>>>>> ---
>>>>> drivers/ata/libahci_platform.c | 13 ++++++++++---
>>>>> 1 file changed, 10 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c
>>>>> index 814804582d1d..8aed7b29c7ab 100644
>>>>> --- a/drivers/ata/libahci_platform.c
>>>>> +++ b/drivers/ata/libahci_platform.c
>>>>> @@ -451,15 +451,22 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev,
>>>>> }
>>>>> }
>>>>>
>>>>> - hpriv->nports = child_nodes = of_get_child_count(dev->of_node);
>>>>> + /*
>>>>> + * Too many sub-nodes most likely means having something wrong with
>>>>> + * the firmware.
>>>>> + */
>>>>> + child_nodes = of_get_child_count(dev->of_node);
>>>>> + if (child_nodes > AHCI_MAX_PORTS) {
>>>>> + rc = -EINVAL;
>>>>> + goto err_out;
>>>>> + }
>>>>>
>>>>> /*
>>>>> * If no sub-node was found, we still need to set nports to
>>>>> * one in order to be able to use the
>>>>> * ahci_platform_[en|dis]able_[phys|regulators] functions.
>>>>> */
>>>>> - if (!child_nodes)
>>>>> - hpriv->nports = 1;
>>>>> + hpriv->nports = child_nodes ?: 1;
>>>>
>>>
>>>> This change is not necessary and makes the code far less easy to read.
>>>
>>> elaborate please. What change? What part of this change makes the code
>>> less easy to read?
>>
>
>> You changed:
>>
>> if (!child_nodes)
>> hpriv->nports = 1;
>>
>> to:
>>
>> hpriv->nports = child_nodes ?: 1;
>>
>> That is the same. So the change is not needed in the first place, and worse,
>> makes the code way harder to read for no good reason.
>
> No, they aren't the same:
> + if (!child_nodes)
> + hpriv->nports = 1;
> and
> + hpriv->nports = child_nodes ?: 1;
> aren't equivalent. The equivalent implementation would be:
> + if (child_nodes)
> + hpriv->nports = child_nodes;
> + else
> + hpriv->nports = 1;
Then use this code. That cryptic C code is hard to read.
>
> As I said in the patchlog, hpriv->nports is updated now only if
> of_get_child_count() returns a valid number of the child nodes,
> ports, which semantically is more correct. In the previous
> implementation it was always set to the number of child nodes
> no matter whether that value was correct or not.
>
> Regarding the ternary operator with omitted operand. Well, it's not
> that rare beast in the kernel:
> $ grep -r "?:" kernel/ drivers/ mm/ fs/ block/ | wc -l
> 699
> But if you insist in it being not that readable, I can replace it with
> more bulky if-else statement. Do you?
Yes please, use the spelled out if/else. I prefer easy to read code rather
than loosing time trying to understand that cryptic C syntax, which I
actually did not know about.
>
> -Sergey
>
>>
>>>
>>> -Sergey
>>>
>>>>
>>>>>
>>>>> hpriv->phys = devm_kcalloc(dev, hpriv->nports, sizeof(*hpriv->phys), GFP_KERNEL);
>>>>> if (!hpriv->phys) {
>>>>
>>>>
>>>> --
>>>> Damien Le Moal
>>>> Western Digital Research
>>
>>
>> --
>> Damien Le Moal
>> Western Digital Research
--
Damien Le Moal
Western Digital Research