In the case of memory allocation failure, no alignment operation is
required.
Signed-off-by: Li zeming <[email protected]>
---
drivers/parisc/iosapic.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/parisc/iosapic.c b/drivers/parisc/iosapic.c
index 3a8c98615634..33de438916d3 100644
--- a/drivers/parisc/iosapic.c
+++ b/drivers/parisc/iosapic.c
@@ -229,7 +229,9 @@ static struct irt_entry *iosapic_alloc_irt(int num_entries)
* 4-byte alignment on 32-bit kernels
*/
a = (unsigned long)kmalloc(sizeof(struct irt_entry) * num_entries + 8, GFP_KERNEL);
- a = (a + 7UL) & ~7UL;
+ if (a)
+ a = (a + 7UL) & ~7UL;
+
return (struct irt_entry *)a;
}
--
2.18.2
On 9/14/22 08:04, Li zeming wrote:
> In the case of memory allocation failure, no alignment operation is
> required.
>
> Signed-off-by: Li zeming <[email protected]>
> ---
> drivers/parisc/iosapic.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/parisc/iosapic.c b/drivers/parisc/iosapic.c
> index 3a8c98615634..33de438916d3 100644
> --- a/drivers/parisc/iosapic.c
> +++ b/drivers/parisc/iosapic.c
> @@ -229,7 +229,9 @@ static struct irt_entry *iosapic_alloc_irt(int num_entries)
> * 4-byte alignment on 32-bit kernels
> */
> a = (unsigned long)kmalloc(sizeof(struct irt_entry) * num_entries + 8, GFP_KERNEL);
> - a = (a + 7UL) & ~7UL;
> + if (a)
> + a = (a + 7UL) & ~7UL;
> +
As you said, the adjustment isn't required, but it's still ok.
So I think the additional "if" isn't necessary and so I'm not
applying your patch.
Anyway, thanks for your help to try to improve the code!
Helge
> return (struct irt_entry *)a;
> }
>
Am Mittwoch, 14. September 2022, 08:18:19 CEST schrieb Helge Deller:
> On 9/14/22 08:04, Li zeming wrote:
> > In the case of memory allocation failure, no alignment operation is
> > required.
> >
> > Signed-off-by: Li zeming <[email protected]>
> > ---
> >
> > drivers/parisc/iosapic.c | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/parisc/iosapic.c b/drivers/parisc/iosapic.c
> > index 3a8c98615634..33de438916d3 100644
> > --- a/drivers/parisc/iosapic.c
> > +++ b/drivers/parisc/iosapic.c
> > @@ -229,7 +229,9 @@ static struct irt_entry *iosapic_alloc_irt(int
> > num_entries)>
> > * 4-byte alignment on 32-bit kernels
> > */
> >
> > a = (unsigned long)kmalloc(sizeof(struct irt_entry) * num_entries
+ 8,
> > GFP_KERNEL);>
> > - a = (a + 7UL) & ~7UL;
> > + if (a)
> > + a = (a + 7UL) & ~7UL;
> > +
>
> As you said, the adjustment isn't required, but it's still ok.
> So I think the additional "if" isn't necessary and so I'm not
> applying your patch.
>
> Anyway, thanks for your help to try to improve the code!
I was about to say the same, but from looking at the code I don't think what
is in there is correct either. The comment seems outdated, because
__assume_kmalloc_alignment, which is __alignof__(unsigned long long). This
code is untouched for the entire git history, so maybe we can just change the
whole thing to
return kcalloc(num_entries, sizeof(struct irt_entry))
now?
And these functions end up propagating an allocation error in this file and it
will never reach kernel/setup.c, which seems bad. But I guess the only point
where this really can go wrong if the PDC returns an absurdly large number of
entries.
Eike
On 9/14/22 08:43, Rolf Eike Beer wrote:
> Am Mittwoch, 14. September 2022, 08:18:19 CEST schrieb Helge Deller:
>> On 9/14/22 08:04, Li zeming wrote:
>>> In the case of memory allocation failure, no alignment operation is
>>> required.
>>>
>>> Signed-off-by: Li zeming <[email protected]>
>>> ---
>>>
>>> drivers/parisc/iosapic.c | 4 +++-
>>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/parisc/iosapic.c b/drivers/parisc/iosapic.c
>>> index 3a8c98615634..33de438916d3 100644
>>> --- a/drivers/parisc/iosapic.c
>>> +++ b/drivers/parisc/iosapic.c
>>> @@ -229,7 +229,9 @@ static struct irt_entry *iosapic_alloc_irt(int
>>> num_entries)>
>>> * 4-byte alignment on 32-bit kernels
>>> */
>>>
>>> a = (unsigned long)kmalloc(sizeof(struct irt_entry) * num_entries
> + 8,
>>> GFP_KERNEL);>
>>> - a = (a + 7UL) & ~7UL;
>>> + if (a)
>>> + a = (a + 7UL) & ~7UL;
>>> +
>>
>> As you said, the adjustment isn't required, but it's still ok.
>> So I think the additional "if" isn't necessary and so I'm not
>> applying your patch.
>>
>> Anyway, thanks for your help to try to improve the code!
>
> I was about to say the same, but from looking at the code I don't think what
> is in there is correct either. The comment seems outdated, because
> __assume_kmalloc_alignment, which is __alignof__(unsigned long long). This
> code is untouched for the entire git history, so maybe we can just change the
> whole thing to
>
> return kcalloc(num_entries, sizeof(struct irt_entry))
>
> now?
Yes, your proposal is good.
Anyone want to send a patch (with a small comment that kcalloc() will return
at least the required 8-byte alignment)?
> And these functions end up propagating an allocation error in this file and it
> will never reach kernel/setup.c, which seems bad.
That part I don't understand.
The return value of iosapic_alloc_irt() is checked afterwards, but you probably
meant something else?
> But I guess the only point where this really can go wrong if the PDC
> returns an absurdly large number of entries.
Helge
Am Mittwoch, 14. September 2022, 11:04:33 CEST schrieb Helge Deller:
> On 9/14/22 08:43, Rolf Eike Beer wrote:
> > Am Mittwoch, 14. September 2022, 08:18:19 CEST schrieb Helge Deller:
> >> On 9/14/22 08:04, Li zeming wrote:
> Yes, your proposal is good.
> Anyone want to send a patch (with a small comment that kcalloc() will return
> at least the required 8-byte alignment)?
Done.
> > And these functions end up propagating an allocation error in this file
> > and it will never reach kernel/setup.c, which seems bad.
>
> That part I don't understand.
> The return value of iosapic_alloc_irt() is checked afterwards, but you
> probably meant something else?
>
> > But I guess the only point where this really can go wrong if the PDC
> > returns an absurdly large number of entries.
What I meant was that if iosapic_alloc_irt() fails, then iosapic_load_irt()
will return 0, which can either be "nothing to do" or "error". iosapic_init()
is void, so even if it could detect the failure, it can't report it upwards to
parisc_init(). Which is the same for basically all other *_init() calls in
there.
Eike
On 9/14/22 16:25, Rolf Eike Beer wrote:
> Am Mittwoch, 14. September 2022, 11:04:33 CEST schrieb Helge Deller:
>> On 9/14/22 08:43, Rolf Eike Beer wrote:
>>> Am Mittwoch, 14. September 2022, 08:18:19 CEST schrieb Helge Deller:
>>>> On 9/14/22 08:04, Li zeming wrote:
>
>> Yes, your proposal is good.
>> Anyone want to send a patch (with a small comment that kcalloc() will return
>> at least the required 8-byte alignment)?
>
> Done.
>
>>> And these functions end up propagating an allocation error in this file
>>> and it will never reach kernel/setup.c, which seems bad.
>>
>> That part I don't understand.
>> The return value of iosapic_alloc_irt() is checked afterwards, but you
>> probably meant something else?
>>
>>> But I guess the only point where this really can go wrong if the PDC
>>> returns an absurdly large number of entries.
>
> What I meant was that if iosapic_alloc_irt() fails, then iosapic_load_irt()
> will return 0, which can either be "nothing to do" or "error". iosapic_init()
> is void, so even if it could detect the failure, it can't report it upwards to
> parisc_init(). Which is the same for basically all other *_init() calls in
> there.
Ok, I see.
Not sure if that needs fixing. If the allocation fails we will be in trouble anyway :-)
Helge