Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 10 Apr 2023 00:38:36 +0200 (CEST)
From:   Jiri Kosina <jikos@kernel.org>
To:     Ding Hui <dinghui@sangfor.com.cn>
cc:     jejb@linux.ibm.com,
        "Martin K. Petersen" <martin.petersen@oracle.com>,
        linux-scsi@vger.kernel.org, linux-kernel@vger.kernel.org,
        Michal Kolar <mich.k@seznam.cz>
Subject: Re: [PATCH] scsi: ses: Handle enclosure with just a primary component
 gracefully
In-Reply-To: <b71bca20-401f-c877-168c-c26ca1c282d1@sangfor.com.cn>
Message-ID: <nycvar.YFH.7.76.2304100037190.29760@cbobk.fhfr.pm>
References: <nycvar.YFH.7.76.2304042122270.29760@cbobk.fhfr.pm> <fa174980e5c9bdafae3426fffe45b97b37f69c84.camel@linux.ibm.com> <b71bca20-401f-c877-168c-c26ca1c282d1@sangfor.com.cn>
User-Agent: Alpine 2.21 (LSU 202 2017-01-01)
MIME-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-15
Content-Transfer-Encoding: 8BIT
Precedence: bulk

On Wed, 5 Apr 2023, Ding Hui wrote:

> >> This reverts 3fe97ff3d9493 ("scsi: ses: Don't attach if enclosure has
> >> no components") and introduces proper handling of case where there
> >> are no detected secondary components, but primary component
> >> (enumerated in num_enclosures) does exist. That fix was originally
> >> proposed by Ding Hui <dinghui@sangfor.com.cn>.
> > 
> > I think everything in here looks fine except this:
> > 
> >> --- a/drivers/scsi/ses.c
> >> +++ b/drivers/scsi/ses.c
> >> @@ -509,9 +509,6 @@ static int ses_enclosure_find_by_addr(struct
> >> enclosure_device *edev,
> >>  ????????int i;
> >>  ????????struct ses_component *scomp;
> >>   -???????if (!edev->component[0].scratch)
> >> -???????????????return 0;
> >> -
> >>  ????????for (i = 0; i < edev->components; i++) {
> >>  ????????????????scomp = edev->component[i].scratch;
> >>  ????????????????if (scomp->addr != efd->addr)
> > 
> > If you remove the check, then scomp could be NULL here and we'll oops
> > on scomp->addr.
> 
> I think we should remove the check, because the edev->components 
> represented the effectiveness of array pointers, so we need check 
> edev->components firstly instead of checking edev->component[0].scratch, 
> if edev->components is 0, we won't enter the for loop, don't worry about 
> dereference scomp.

Right you are. So v1 is actually more correct.

Martin, could you please consider adding

	Tested-by: Michal Kolar <mich.k@seznam.cz>

to v1 and applying? Thanks,

-- 
Jiri Kosina
SUSE Labs