Re: [PATCH 04/20] [SCSI] mpt3sas: Remove redundancy code while freeing the controller resources.

[Sreekanth Reddy]

Hi,

Any other review comments on this patch. please let us known if any
changes are required.

Thanks,
Sreekanth

On Mon, Jun 15, 2015 at 4:18 PM, Johannes Thumshirn <jthumshirn@xxxxxxx> wrote:
> On Mon, Jun 15, 2015 at 03:56:56PM +0530, Sreekanth Reddy wrote:
>> On Fri, Jun 12, 2015 at 6:10 PM, Johannes Thumshirn <jthumshirn@xxxxxxx> wrote:
>> > On Fri, Jun 12, 2015 at 05:48:56PM +0530, Sreekanth Reddy wrote:
>> >> On Fri, Jun 12, 2015 at 4:58 PM, Johannes Thumshirn <jthumshirn@xxxxxxx> wrote:
>> >> > On Fri, Jun 12, 2015 at 03:12:16PM +0530, Sreekanth Reddy wrote:
>> >> >> Removed the redundancy code while freeing the controller resources.
>> >> >>
>> >> >> Signed-off-by: Sreekanth Reddy <Sreekanth.Reddy@xxxxxxxxxxxxx>
>> >> >> ---
>> >> >>  drivers/scsi/mpt3sas/mpt3sas_base.c | 57 +++++++++++++++++++++----------------
>> > [...]
>> >> >> +     _base_free_irq(ioc);
>> >> >> +     _base_disable_msix(ioc);
>> >> >> +
>> >> >> +     if (ioc->msix96_vector)
>> >> >> +             kfree(ioc->replyPostRegisterIndex);
>> >> >
>> >> > kfree() already checks for zero or a NULL pointer.
>> >>
>> >> Sorry Johannes, I didn't get you. If I understand this correctly, you
>> >> are suggesting to check for NULL pointer before calling kree() API as
>> >> shown below,
>> >>
>> >> if (ioc->msix96_vector && (ioc->replyPostRegisterIndex != NULL))
>> >>         kfree(ioc->replyPostRegisterIndex);
>> >
>> > Correct me if I'm wrong, but I thought you don't need the if
>> > (ioc->msix96_vector) before the kfree(). ioc->replyPostRegisterIndex should be
>> > NULL if ioc->msix96_vector is 0, as far as I can see.
>> >
>> > In _scsih_probe() you have:
>> > shost = scsi_host_alloc(&scsih_driver_template,
>> >             sizeof(struct MPT3SAS_ADAPTER));
>> > if (!shost)
>> >            return -ENODEV;
>> >
>> > /* init local params */
>> > ioc = shost_priv(shost);
>> >
>> > and scsi_host_alloc() does a kzalloc() for shost.
>> >
>> > so ioc->replyPortRegisterIndex is NULL.
>> >
>> > Or am I thinking wrong here?
>>
>> Yes, ioc->replyPostRegisterIndex will be NULL if ioc->msix96_vector is 0,
>> We have added this checks as a precautionary. since calling this
>> function (i.e kfree()) on memory not previously allocated with
>> kmalloc(), or on memory which has already been freed, may results in
>> very bad things, such as freeing memory belonging to another part of
>> the kernel.
>>
>
> OK, then please leave it in.
>
>> >
>> >>
>> >> Regards,
>> >> Sreekanth
>> >
>> > --
>> > Johannes Thumshirn                                       Storage
>> > jthumshirn@xxxxxxx                             +49 911 74053 689
>> > SUSE LINUX GmbH, Maxfeldstr. 5, 90409 NÃrnberg
>> > GF: F. ImendÃrffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
>> > HRB 21284 (AG NÃrnberg)
>>
>>
>>
>> --
>>
>> Regards,
>> Sreekanth
>
> --
> Johannes Thumshirn                                       Storage
> jthumshirn@xxxxxxx                             +49 911 74053 689
> SUSE LINUX GmbH, Maxfeldstr. 5, 90409 NÃrnberg
> GF: F. ImendÃrffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
> HRB 21284 (AG NÃrnberg)



-- 

Regards,
Sreekanth
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/