From owner-freebsd-current@FreeBSD.ORG Thu Nov 17 18:25:10 2011 Return-Path: Delivered-To: current@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 58C6C106567B; Thu, 17 Nov 2011 18:25:10 +0000 (UTC) (envelope-from maksim.yevmenkin@gmail.com) Received: from mail-yw0-f54.google.com (mail-yw0-f54.google.com [209.85.213.54]) by mx1.freebsd.org (Postfix) with ESMTP id 7B5048FC14; Thu, 17 Nov 2011 18:25:03 +0000 (UTC) Received: by ywe9 with SMTP id 9so2119308ywe.13 for ; Thu, 17 Nov 2011 10:25:03 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=mime-version:sender:in-reply-to:references:date :x-google-sender-auth:message-id:subject:from:to:cc:content-type :content-transfer-encoding; bh=nIfnnM98luUkP9aUadIkLRdF5RrYeoKHv3z5dwmPh5o=; b=ctObwYgcORBdbP+7Z8LQo/ITbO7VC4ToRVJYr7vIVoK2s0HoZx+ePNQXOydkmqJQfN L3zy/Ue746YQMO2zzRBeKXwRSwitgC1SqTBBKbLvz1hL2Zf30Lp1TPbHjWbc/714TRM9 GV+cpEATPkLw40cWHweXjL5u3ng865BmPKdOM= MIME-Version: 1.0 Received: by 10.236.22.136 with SMTP id t8mr11805922yht.30.1321554302698; Thu, 17 Nov 2011 10:25:02 -0800 (PST) Sender: maksim.yevmenkin@gmail.com Received: by 10.100.122.19 with HTTP; Thu, 17 Nov 2011 10:25:02 -0800 (PST) In-Reply-To: <4EC54624.4000409@FreeBSD.org> References: <4EC43ABA.7060407@FreeBSD.org> <4EC4404B.4070008@FreeBSD.org> <4EC531A4.4000803@FreeBSD.org> <4EC53E28.2030609@FreeBSD.org> <4EC54624.4000409@FreeBSD.org> Date: Thu, 17 Nov 2011 10:25:02 -0800 X-Google-Sender-Auth: e7olkSEmUsH0JXy3RYUuiWpWYNQ Message-ID: From: Maksim Yevmenkin To: Alexander Motin Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Cc: current@freebsd.org Subject: Re: [RFC] ahci(4) patch X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 17 Nov 2011 18:25:10 -0000 On Thu, Nov 17, 2011 at 9:36 AM, Alexander Motin wrote: > On 11/17/11 19:02, Alexander Motin wrote: >> On 11/17/11 18:35, Maksim Yevmenkin wrote: >>> On Thu, Nov 17, 2011 at 8:09 AM, Alexander Motin wrot= e: >>>> On 11/17/11 01:08, Maksim Yevmenkin wrote: >>>>> On Wed, Nov 16, 2011 at 2:59 PM, Alexander Motin wr= ote: >>>>>> On 17.11.2011 00:44, Maksim Yevmenkin wrote: >>>>>>> >>>>>>> On Wed, Nov 16, 2011 at 2:35 PM, Alexander Motin = =A0wrote: >>>>>>>> >>>>>>>> On 16.11.2011 23:59, Maksim Yevmenkin wrote: >>>>>>>>> >>>>>>>>> would anyone object to the following ahci(4) patch? >>>>>>>>> >>>>>>>>> =3D=3D >>>>>>>>> >>>>>>>>> --- ahci.c.orig 2011-11-16 21:35:26.000000000 +0000 >>>>>>>>> +++ ahci.c =A0 =A0 =A02011-11-16 21:35:41.000000000 +0000 >>>>>>>>> @@ -500,7 +500,7 @@ >>>>>>>>> =A0 =A0 =A0 =A0for (unit =3D 0; unit< =A0 =A0ctlr->channels; unit= ++) { >>>>>>>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0if ((ctlr->ichannels& =A0 =A0(1<< = =A0 =A0unit)) =3D=3D 0) >>>>>>>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0continue; >>>>>>>>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 child =3D device_add_child(dev, "ah= cich", -1); >>>>>>>>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 child =3D device_add_child(dev, "ah= cich", unit); >>>>>>>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0if (child =3D=3D NULL) >>>>>>>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0device_printf(dev,= "failed to add channel >>>>>>>>> device\n"); >>>>>>>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0else >>>>>>>>> >>>>>>>>> =3D=3D >>>>>>>>> >>>>>>>>> the idea is to have "static" numbering for ada(4) disks. >>>>>>>> >>>>>>>> I do. The only way I see this useful is if you have BIOS configure= d for >>>>>>>> non-hot-swappable disks, in which case you have some AHCI channels >>>>>>>> disabled, >>>>>>>> but want to keep numbers of the rest. While I don't like this mode= in >>>>>>>> general, especially when it can't be disabled, that patch could be= useful >>>>>>>> in >>>>>>>> these cases. But in other cases, when you have several AHCI contro= llers, >>>>>>>> it >>>>>>>> just wont not work. You will receive error on attempt to create se= cond >>>>>>>> ahcich0. >>>>>>> >>>>>>> shouldn't achcichX be destroyed when disk is detached/removed/etc.? >>>>>> >>>>>> List of implemented AHCI channels to expose as ahcichX set by BIOS i= n >>>>>> vendor-specific way, but only during boot and not by many BIOSes. De= stroying >>>>>> them on disk detach theoretically possible, but IMHO not right, as b= us >>>>>> doesn't disappear on disk disconnect. >>>>>> >>>>>>> the particular problem i'm trying to address is disk re-numbering w= hen >>>>>>> one of the disks fails/removed/etc. i'm trying to use hints to wire >>>>>>> disks to controllers/busses. it works perfectly fine with da(4) dis= ks >>>>>>> (even hot swappable ones) , but i can not make it to work with ada(= 4) >>>>>>> disks. i'm perfectly fine to hid this under some sort of option >>>>>>> (something similar to ATA_STATIC_ID, AHCI_STATIC_ID for example) >>>>>> >>>>>> Wiring works for adaX also, unless your BIOS is so "intelligent" to = report >>>>>> unconnected ports and not impliemented.. You should just wire CAM bu= ses to >>>>>> ahcichX, not to ahciX. It could be done in other way changing just b= y one >>>>>> line around xpt_bus_register(), but now it is done so. >>>>> >>>>> ok. then i must be missing something, here is what i have in device.h= ints >>>>> >>>>> hint.scbus.0.at=3D"umass-sim0" >>>>> hint.scbus.1.at=3D"ahcich0" >>>>> hint.scbus.2.at=3D"ahcich1" >>>>> hint.scbus.3.at=3D"ahcich2" >>>>> hint.scbus.4.at=3D"ahcich3" >>>>> hint.scbus.5.at=3D"ahcich4" >>>>> hint.scbus.6.at=3D"ahcich5" >>>>> >>>>> hint.da.0.at=3D"scbus0" >>>>> hint.ada.0.at=3D"scbus1" >>>>> hint.ada.1.at=3D"scbus2" >>>>> hint.ada.2.at=3D"scbus3" >>>>> hint.ada.3.at=3D"scbus4" >>>>> hint.ada.4.at=3D"scbus5" >>>>> hint.ada.5.at=3D"scbus6" >>>>> >>>>> this is for 6-port ahci(4) compatible controller (intel) on the >>>>> motherboard. no matter which achi(4) ports are connected, resulted >>>>> adaX devices are always sequential starting from ada0. so, the >>>>> question is: what am i doing wrong here? >>>> >>>> Just put your lines into my loader.conf and got this: >>>> >>>> %camcontrol devlist -v >>>> scbus1 on ahcich0 bus 0: >>>> =A0 =A0at scbus1 target 0 lun 0 (pass0= ,ada0) >>>> <> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 at = scbus1 target -1 lun -1 () >>>> scbus2 on ahcich1 bus 0: >>>> <> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 at = scbus2 target -1 lun -1 () >>>> scbus3 on ahcich2 bus 0: >>>> =A0 =A0at scbus3 target 0 lun 0 (pass1= ,ada2) >>>> <> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 at = scbus3 target -1 lun -1 () >>>> scbus4 on ahcich3 bus 0: >>>> <> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 at = scbus4 target -1 lun -1 () >>>> scbus5 on ahcich4 bus 0: >>>> <> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 at = scbus5 target -1 lun -1 () >>>> scbus6 on ahcich5 bus 0: >>>> <> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 at = scbus6 target -1 lun -1 () >>>> ... >>>> >>>> I see no problem. What am I doing wrong? >>>> >>>> Let me repeat question not asked directly: Does your BIOS reports unus= ed >>>> AHCI channels as implemented? Do you always have all 6 ahcich devices = or >>>> not? >>> >>> i not exactly sure. below is the relevant dmesg. as far as i can tell, >>> channels are correct, however, ahcich numbering is sequential and that >>> is why hints dont work. >>> >>> Nov 17 09:22:51 PREPROD-red1 kernel: ahci0: >> SATA controller> port >>> 0xf070-0xf077,0xf060-0xf063,0xf050-0xf057,0xf040-0xf043,0xf000-0xf01f >>> mem 0xfbb21000-0xfbb217ff irq 19 at device 31.2 on pci0 >>> Nov 17 09:22:51 PREPROD-red1 kernel: ahci0: attempting to allocate 1 >>> MSI vectors (1 supported) >>> Nov 17 09:22:51 PREPROD-red1 kernel: ahci0: using IRQ 270 for MSI >>> Nov 17 09:22:51 PREPROD-red1 kernel: ahci0: AHCI v1.30 with 6 6Gbps >>> ports, Port Multiplier not supported >>> Nov 17 09:22:51 PREPROD-red1 kernel: ahci0: Caps: 64bit NCQ SNTF AL >>> CLO 6Gbps PMD SSC PSC 32cmd EM 6ports >>> Nov 17 09:22:51 PREPROD-red1 kernel: ahci0: Caps2: APST >>> Nov 17 09:22:51 PREPROD-red1 kernel: ahci0: EM Caps: ALHD XMT SMB LED >>> Nov 17 09:22:51 PREPROD-red1 kernel: ahcich0: at >>> channel 0 on ahci0 >>> Nov 17 09:22:51 PREPROD-red1 kernel: ahcich0: Caps: >>> Nov 17 09:22:51 PREPROD-red1 kernel: ahcich1: at >>> channel 2 on ahci0 >>> Nov 17 09:22:51 PREPROD-red1 kernel: ahcich1: Caps: >>> Nov 17 09:22:51 PREPROD-red1 kernel: ahcich2: at >>> channel 5 on ahci0 >>> Nov 17 09:22:51 PREPROD-red1 kernel: ahcich2: Caps: >> >> Yes, that's it. You should have six ahcichX devices, but have only three >> - only for connected devices. Some vendors (Supermicro was first I've >> heard about) last time randomly started to make BIOS mark unused AHCI >> channels as unimplemented. Sometimes it can be configured in BIOS setup >> (it may be called like "hot-swappable"), sometimes not. I believe that >> is wrong AHCI spec interpretation. >> >> If it is not configurable in BIOS, we could add respective hint to the >> ahci(4) to allow user ignore these fake "implemented" flags to always >> present all possible ports, but it is direct AHCI spec violation. >> >> Other possible way is to leave ahcichX device as-is, but instead >> register ports in CAM in different fashion -- as buses of the same >> controller. Patch may look like this: >> >> --- ahci.c =A0 =A0 =A0(revision 227580) >> +++ ahci.c =A0 =A0 =A0(working copy) >> @@ -985,8 +985,8 @@ >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 goto err1; >> =A0 =A0 =A0 =A0 } >> =A0 =A0 =A0 =A0 /* Construct SIM entry */ >> - =A0 =A0 =A0 ch->sim =3D cam_sim_alloc(ahciaction, ahcipoll, "ahcich", = ch, >> - =A0 =A0 =A0 =A0 =A0 device_get_unit(dev), &ch->mtx, >> + =A0 =A0 =A0 ch->sim =3D cam_sim_alloc(ahciaction, ahcipoll, "ahci", ch= , >> + =A0 =A0 =A0 =A0 =A0 device_get_unit(device_get_parent(dev)), &ch->mtx, >> =A0 =A0 =A0 =A0 =A0 =A0 min(2, ch->numslots), >> =A0 =A0 =A0 =A0 =A0 =A0 (ch->caps & AHCI_CAP_SNCQ) ? ch->numslots : 0, >> =A0 =A0 =A0 =A0 =A0 =A0 devq); >> @@ -996,7 +996,7 @@ >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 error =3D ENOMEM; >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 goto err1; >> =A0 =A0 =A0 =A0 } >> - =A0 =A0 =A0 if (xpt_bus_register(ch->sim, dev, 0) !=3D CAM_SUCCESS) { >> + =A0 =A0 =A0 if (xpt_bus_register(ch->sim, dev, ch->unit) !=3D CAM_SUCC= ESS) { >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 device_printf(dev, "unable to register x= pt bus\n"); >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 error =3D ENXIO; >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 goto err2; >> >> Then output looks that way: >> %camcontrol devlist -v >> scbus1 on ahci0 bus 0: >> =A0 =A0at scbus1 target 0 lun 0 (pass0,a= da0) >> <> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 at sc= bus1 target -1 lun -1 () >> scbus2 on ahci0 bus 1: >> <> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 at sc= bus2 target -1 lun -1 () >> scbus3 on ahci0 bus 2: >> =A0 =A0at scbus3 target 0 lun 0 (pass1,a= da2) >> <> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 at sc= bus3 target -1 lun -1 () >> scbus4 on ahci0 bus 3: >> <> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 at sc= bus4 target -1 lun -1 () >> scbus5 on ahci0 bus 4: >> <> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 at sc= bus5 target -1 lun -1 () >> scbus6 on ahci0 bus 5: >> <> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 at sc= bus6 target -1 lun -1 () >> >> But in that case two problems remain: >> =A0- per-port device hints still won't be usable, while changing them wi= ll >> break POLA; >> =A0- it will also break POLA for users who are using wiring now. > > One more idea. We can create ahcichX devices for every AHCI channel, but > disable them for channels marked as not implemented: > > --- ahci.c =A0 =A0 =A0(revision 227580) > +++ ahci.c =A0 =A0 =A0(working copy) > @@ -498,13 +498,14 @@ > =A0 =A0 =A0 =A0} > =A0 =A0 =A0 =A0/* Attach all channels on this controller */ > =A0 =A0 =A0 =A0for (unit =3D 0; unit < ctlr->channels; unit++) { > - =A0 =A0 =A0 =A0 =A0 =A0 =A0 if ((ctlr->ichannels & (1 << unit)) =3D=3D = 0) > - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 continue; > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0child =3D device_add_child(dev, "ahcich", = -1); > - =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (child =3D=3D NULL) > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (child =3D=3D NULL) { > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0device_printf(dev, "failed= to add channel > device\n"); > - =A0 =A0 =A0 =A0 =A0 =A0 =A0 else > - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 device_set_ivars(child, (vo= id *)(intptr_t)unit); > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 continue; > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 } > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 device_set_ivars(child, (void *)(intptr_t)u= nit); > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if ((ctlr->ichannels & (1 << unit)) =3D=3D = 0) > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 device_disable(child); > =A0 =A0 =A0 =A0} > =A0 =A0 =A0 =A0bus_generic_attach(dev); > =A0 =A0 =A0 =A0return 0; > > Device disabling is not very used functionality now, but it seems fit > perfectly for this case: > > ahcich0: at channel 0 on ahci0 > ahcich0: Caps: HPCP > ahcich1: not probed (disabled) > ahcich2: at channel 2 on ahci0 > ahcich2: Caps: HPCP > ahcich3: at channel 3 on ahci0 > ahcich3: Caps: HPCP > ahcich4: not probed (disabled) > ahcich5: not probed (disabled) it looks fine as long as i can wire ada(4) disks to prevent re-ordering thanks max