Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752590AbbGUHFL (ORCPT ); Tue, 21 Jul 2015 03:05:11 -0400 Received: from mx0b-00082601.pphosted.com ([67.231.153.30]:47447 "EHLO mx0b-00082601.pphosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752352AbbGUHFJ (ORCPT ); Tue, 21 Jul 2015 03:05:09 -0400 Date: Tue, 21 Jul 2015 00:04:32 -0700 From: Calvin Owens To: Joe Lawrence CC: Nagalakshmi Nandigama , Praveen Krishnamoorthy , Sreekanth Reddy , Abhijit Mahajan , , , , , Christoph Hellwig , Bart Van Assche Subject: Re: [PATCH 1/2] mpt2sas: Refcount sas_device objects and fix unsafe list usage Message-ID: <20150721070432.GA1353000@mail.thefacebook.com> References: <1433821856-2815280-1-git-send-email-calvinowens@fb.com> <1436675096-324527-1-git-send-email-calvinowens@fb.com> <1436675096-324527-2-git-send-email-calvinowens@fb.com> <55A3D3A5.6090005@stratus.com> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline In-Reply-To: <55A3D3A5.6090005@stratus.com> User-Agent: Mutt/1.5.20 (2009-12-10) X-Originating-IP: [192.168.52.123] X-Proofpoint-Spam-Reason: safe X-FB-Internal: Safe X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:5.14.151,1.0.33,0.0.0000 definitions=2015-07-21_02:2015-07-20,2015-07-21,1970-01-01 signatures=0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3293 Lines: 83 On Monday 07/13 at 11:05 -0400, Joe Lawrence wrote: > On 07/12/2015 12:24 AM, Calvin Owens wrote: > > These objects can be referenced concurrently throughout the driver, we > > need a way to make sure threads can't delete them out from under each > > other. This patch adds the refcount, and refactors the code to use it. > > > > Additionally, we cannot iterate over the sas_device_list without > > holding the lock, or we risk corrupting random memory if items are > > added or deleted as we iterate. This patch refactors _scsih_probe_sas() > > to use the sas_device_list in a safe way. > > > > Cc: Christoph Hellwig > > Cc: Bart Van Assche > > Signed-off-by: Calvin Owens > > --- > > drivers/scsi/mpt2sas/mpt2sas_base.h | 22 +- > > drivers/scsi/mpt2sas/mpt2sas_scsih.c | 434 ++++++++++++++++++++----------- > > drivers/scsi/mpt2sas/mpt2sas_transport.c | 12 +- > > 3 files changed, 315 insertions(+), 153 deletions(-) > > [ ... snip ... ] > > > @@ -2078,7 +2150,7 @@ _scsih_slave_configure(struct scsi_device *sdev) > > } > > > > spin_lock_irqsave(&ioc->sas_device_lock, flags); > > - sas_device = mpt2sas_scsih_sas_device_find_by_sas_address(ioc, > > + sas_device = __mpt2sas_get_sdev_by_addr(ioc, > > sas_device_priv_data->sas_target->sas_address); > > if (!sas_device) { > > spin_unlock_irqrestore(&ioc->sas_device_lock, flags); > > @@ -2116,13 +2188,14 @@ _scsih_slave_configure(struct scsi_device *sdev) > > if (!ssp_target) > > _scsih_display_sata_capabilities(ioc, handle, sdev); > > > > - > > _scsih_change_queue_depth(sdev, qdepth); > > > > if (ssp_target) { > > sas_read_port_mode_page(sdev); > > _scsih_enable_tlr(ioc, sdev); > > } > > + > > + sas_device_put(sas_device); > > return 0; > > } > > Hi Calvin, > > Any reason why this sas_device_put is placed outside the sas_device > lock? Most other instances in this patch were called just before unlocking. Thanks for looking at this. I guess I thought that something below where we drop the sas_device_lock referenced it, but it looks like nothing does. I'll move it up in v3. I don't think it's strictly necessary that the put() happen under the lock: the only way this could be the final put() is if both ->hostdata and the sas_device_list had dropped their references, and in that case it would be impossible to have a concurrent get(), since those are the only two ways to lookup/get a sas_device. But absent any reason not to, let's make it more consistent. I'm really glad you pointed this out, because I realized I flubbed this in _scsih_target_alloc() and forgot to eliminate the sas_device_put() from before the ->hostdata lookup was added. I'll fix this in v3. > BTW I attempted testing, but needed to port to mpt3 and ended up with a > driver that didn't boot :( Hopefully I can retry later this week, or > find an older mpt2 box lying around. More testing would be fantastic if that's possible :) Thanks very much, Calvin > -- Joe -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/