Re: [RFC PATCH v2 5/5] bnx2fc: Use the fcoe_sysfs control interface
On 09/26/2012 07:02 PM, Robert Love wrote:
This patch adds support for the new fcoe_sysfs
control interface to bnx2fc.ko. It keeps the deprecated
interface in tact and therefore either the legacy
or the new control interfaces can be used. A mixed mode
is not supported. A user must either use the new
interfaces or the old ones, but not both.
The fcoe_ctlr's link state is now driven by both the
netdev link state as well as the fcoe_ctlr_device's
enabled attribute. The link must be up and the
fcoe_ctlr_device must be enabled before the FCoE
Controller starts discovery or login.
Signed-off-by: Robert Love
Changes look good to me. I did some testing and did not observe any
issues, except that a small modification is required in _bnx2fc_create()
(see below). In fact, I like the new design, as we do not have any user
space compatibility issues.
I also see that even the non-netdev based FCoE driver can also take
advantage of this provided they use libfcoe for FIP, which is good.
Thanks.
+/**
+ * _bnx2fc_create() - Create bnx2fc FCoE interface
+ * @netdev : The net_device object the Ethernet interface to create on
+ * @fip_mode: The FIP mode for this creation
+ * @link_state: The ctlr link state on creation
*
- * Called from sysfs.
+ * Called from either the libfcoe 'create' module parameter
+ * via fcoe_create or from fcoe_syfs's ctlr_create file.
+ *
+ * libfcoe's 'create' module parameter is deprecated so some
+ * consolidation of code can be done when that interface is
+ * removed.
*
* Returns: 0 for success
*/
-static int bnx2fc_create(struct net_device *netdev, enum fip_state fip_mode)
+static int _bnx2fc_create(struct net_device *netdev,
+ enum fip_state fip_mode,
+ enum bnx2fc_create_link_state link_state)
{
+ struct fcoe_ctlr_device *cdev;
struct fcoe_ctlr *ctlr;
struct bnx2fc_interface *interface;
struct bnx2fc_hba *hba;
@@ -2111,7 +2177,15 @@ static int bnx2fc_create(struct net_device *netdev, enum
fip_state fip_mode)
/* Make this master N_port */
ctlr->lp = lport;
- if (!bnx2fc_link_ok(lport)) {
+ cdev = fcoe_ctlr_to_ctlr_dev(ctlr);
+
+ if (link_state == BNX2FC_CREATE_LINK_UP)
+ cdev->enabled = FCOE_CTLR_ENABLED;
+ else
+ cdev->enabled = FCOE_CTLR_DISABLED;
+
+ if (link_state == BNX2FC_CREATE_LINK_UP &&
+ !bnx2fc_link_ok(lport)) {
fcoe_ctlr_link_up(ctlr);
fc_host_port_type(lport->host) = FC_PORTTYPE_NPORT;
set_bit(ADAPTER_STATE_READY, &interface->hba->adapter_state);
bnx2fc needs the following check in _bnx2fc_create. Otherwise, during
ifup, bnx2fc_ulp_start() may start the interface even if enabled is not set.
diff --git a/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
b/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
index 60baf88..e96bf54 100644
--- a/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
+++ b/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
@@ -2193,7 +2193,8 @@ static int _bnx2fc_create(struct net_device *netdev,
BNX2FC_HBA_DBG(lport, "create: START DISC\n");
bnx2fc_start_disc(interface);
- interface->enabled = true;
+ if (link_state == BNX2FC_CREATE_LINK_UP)
+ interface->enabled = true;
/*
* Release from kref_init in bnx2fc_interface_setup, on success
* lport should be holding a reference taken in bnx2fc_if_create
@@ -2145,6 +2219,37 @@ mod_err:
}
/**
+ * bnx2fc_create() - Create a bnx2fc interface
+ * @netdev : The net_device object the Ethernet interface to create on
+ * @fip_mode: The FIP mode for this creation
+ *
+ * Called from fcoe transport
+ *
+ * Returns: 0 for success
+ */
+static int bnx2fc_create(struct net_device *netdev, enum fip_state fip_mode)
+{
+ return _bnx2fc_create(netdev, fip_mode, BNX2FC_CREATE_LINK_UP);
+}
+
+/**
+ * bnx2fc_ctlr_alloc() - Allocate a bnx2fc interface from fcoe_sysfs
+ * @netdev: The net_device to be used by the allocated FCoE Controller
+ *
+ * This routine is called from fcoe_sysfs. It will start the fcoe_ctlr
+ * in a link_down state. The allows the user an opportunity to configure
+ * the FCoE Controller from sysfs before enabling the FCoE Controller.
+ *
+ * Creating in with this routine starts the FCoE Controller in Fabric
+ * mode. The user can change to VN2VN or another mode before enabling.
+ */
+static int bnx2fc_ctlr_alloc(struct net_device *netdev)
+{
+ return _bnx2fc_create(netdev, FIP_MODE_FABRIC,
+ BNX2FC_CREATE_LINK_DOWN);
+}
+
+/**
* bnx2fc_find_hba_for_cnic - maps cnic instance to bnx2fc hba instance
*
* @cnic: Pointer to cnic device instance
@@ -2270,6 +2375,7 @@ static struct fcoe_transport bnx2fc_transport = {
.name = {"bnx2fc"},
.attached = false,
.list = LIST_HEAD_INIT(bnx2fc_transport.list),
+ .alloc = bnx2fc_ctlr_alloc,
.match = bnx2fc_match,
.create = bnx
Re: [PATCH v7 2/6] scsi: sr: support runtime pm
On 09/30/2012 03:43 PM, Alan Stern wrote: On Sun, 30 Sep 2012, Jeff Garzik wrote: The simple fact of "only ZPODD devices out there are ATA" is not the decision-maker for where the code should live. It is more a question where ZPODD belongs in the device/command set model currently employed. I don't really accept this argument. It's a little like saying: The tty layer uses ioctl commands to control RS232 line settings; therefore RS232 settings should be handled in the VFS layer as part of the ioctl core. Hardly -- sr is an optical device driver, much more aligned. And libata is probably at least 50% of the entire sr userbase, if not much much more. Jeff -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [V5 PATCH 0/8] csiostor: Chelsio FCoE offload driver submission
Hi James, Do you have any update with regards to the review of this driver? You were mentioning about getting FC folks to take a look at it. Thanks, Naresh. On 9/24/2012 10:47 PM, Naresh Kumar Inna wrote: > This is the initial submission of the Chelsio FCoE offload driver (csiostor) > to the upstream kernel. This driver currently supports FCoE offload > functionality over Chelsio T4-based 10Gb Converged Network Adapters. > > The following patches contain the driver sources for csiostor driver and > updates to firmware/hardware header files shared between csiostor, > cxgb4 (Chelsio T4-based NIC driver) and cxgb4vf (Chelsio T4-based Virtual > Function NIC driver). The csiostor driver is dependent on these > header updates. These patches have been generated against scsi 'misc' branch. > > csiostor is a low level SCSI driver that interfaces with PCI, SCSI midlayer > and > FC transport subsystems. This driver claims the FCoE PCIe function on > Chelsio Converged Network Adapters. It relies on firmware events for slow path > operations like discovery, thereby offloading session management. The driver > programs firmware via Work Request interfaces for fast path I/O offload > features. > > Version V5 of the patches addresses James's comment, primary among them being > moving to the lockless version of queuecommand(). Rest of the V5 changes are > listed in the individual patches. > > Here is the brief description of patches: > [V5 PATCH 1/8]: Updates to header files shared between cxgb4, cxgb4vf and > csiostor. > [V5 PATCH 2/8]: Header files part 1. > [V5 PATCH 3/8]: Header files part 2. > [V5 PATCH 4/8]: Driver initialization and Work Request services. > [V5 PATCH 5/8]: FC transport interfaces and mailbox services. > [V5 PATCH 6/8]: Local and remote port state tracking functionality. > [V5 PATCH 7/8]: Interrupt handling and fast path I/O functionality. > [V5 PATCH 8/8]: Hardware interface, Makefile and Kconfig changes. > > Naresh Kumar Inna (8): > cxgb4/cxgb4vf: Chelsio FCoE offload driver submission (common header > updates). > csiostor: Chelsio FCoE offload driver submission (headers part 1). > csiostor: Chelsio FCoE offload driver submission (headers part 2). > csiostor: Chelsio FCoE offload driver submission (sources part 1). > csiostor: Chelsio FCoE offload driver submission (sources part 2). > csiostor: Chelsio FCoE offload driver submission (sources part 3). > csiostor: Chelsio FCoE offload driver submission (sources part 4). > csiostor: Chelsio FCoE offload driver submission (sources part 5). > > drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c |2 +- > drivers/net/ethernet/chelsio/cxgb4/sge.c| 10 +- > drivers/net/ethernet/chelsio/cxgb4/t4_hw.c | 16 +- > drivers/net/ethernet/chelsio/cxgb4/t4_msg.h |1 + > drivers/net/ethernet/chelsio/cxgb4/t4_regs.h| 69 +- > drivers/net/ethernet/chelsio/cxgb4/t4fw_api.h | 104 +- > drivers/net/ethernet/chelsio/cxgb4vf/sge.c | 11 +- > drivers/scsi/Kconfig|1 + > drivers/scsi/Makefile |1 + > drivers/scsi/csiostor/Kconfig | 19 + > drivers/scsi/csiostor/Makefile | 11 + > drivers/scsi/csiostor/csio_attr.c | 804 + > drivers/scsi/csiostor/csio_defs.h | 121 + > drivers/scsi/csiostor/csio_hw.c | 4395 > +++ > drivers/scsi/csiostor/csio_hw.h | 666 > drivers/scsi/csiostor/csio_init.c | 1274 +++ > drivers/scsi/csiostor/csio_init.h | 158 + > drivers/scsi/csiostor/csio_isr.c| 624 > drivers/scsi/csiostor/csio_lnode.c | 2133 +++ > drivers/scsi/csiostor/csio_lnode.h | 255 ++ > drivers/scsi/csiostor/csio_mb.c | 1769 + > drivers/scsi/csiostor/csio_mb.h | 278 ++ > drivers/scsi/csiostor/csio_rnode.c | 889 + > drivers/scsi/csiostor/csio_rnode.h | 141 + > drivers/scsi/csiostor/csio_scsi.c | 2554 + > drivers/scsi/csiostor/csio_scsi.h | 342 ++ > drivers/scsi/csiostor/csio_wr.c | 1632 + > drivers/scsi/csiostor/csio_wr.h | 512 +++ > drivers/scsi/csiostor/t4fw_api_stor.h | 578 +++ > 29 files changed, 19333 insertions(+), 37 deletions(-) > create mode 100644 drivers/scsi/csiostor/Kconfig > create mode 100644 drivers/scsi/csiostor/Makefile > create mode 100644 drivers/scsi/csiostor/csio_attr.c > create mode 100644 drivers/scsi/csiostor/csio_defs.h > create mode 100644 drivers/scsi/csiostor/csio_hw.c > create mode 100644 drivers/scsi/csiostor/csio_hw.h > create mode 100644 drivers/scsi/csiostor/csio_init.c > create mode 100644 drivers/scsi/csiostor/csio_init.h > create mode 100644 drivers/scsi/csiostor/csio_isr.c > create mode 100644 drive
Re: [PATCH] qla2xxx: silence two GCC warnings
Am Sonntag 30 September 2012, 13:07:54 schrieb Paul Bolle: > Compiling qla_gs.o (part of the qla2xxx module) triggers two GCC > warnings: > drivers/scsi/qla2xxx/qla_gs.c: In function ‘qla2x00_fdmi_rhba’: > drivers/scsi/qla2xxx/qla_gs.c:1339:7: warning: array subscript is above > array bounds [-Warray-bounds] drivers/scsi/qla2xxx/qla_gs.c: In function > ‘qla2x00_fdmi_register’: drivers/scsi/qla2xxx/qla_gs.c:1663:15: warning: > array subscript is above array bounds [-Warray-bounds] > > It seems that the sequence of a strcpy followed by a strlen confuses GCC > when it is keeping track of array bounds here. (It is not clear to me > which array triggers this warning and by how much GCC thinks the > subscript is above its bounds. Neither is it clear to me why comparable > code in these two functions doesn't trigger this warning.) > > The easiest way to silence these warnings is to hardcode the length of > these two strings in the code here. The length used here is the length > of the string, including its NUL terminator, rounded up to the next > multiple of four. This adds some magic values, which is asking for trouble once someone changes the manufacturer string or something. What about something like this: const char *qlogic = "QLogic Corporation"; strcpy(eiter->a.manufacturer, qlogic); alen += round_up(strlen(qlogic), 4); ... Eike signature.asc Description: This is a digitally signed message part.
Re: [PATCH v7 2/6] scsi: sr: support runtime pm
On Sun, 30 Sep 2012, Jeff Garzik wrote: > The simple fact of "only ZPODD devices out there are ATA" is not the > decision-maker for where the code should live. It is more a question > where ZPODD belongs in the device/command set model currently employed. I don't really accept this argument. It's a little like saying: The tty layer uses ioctl commands to control RS232 line settings; therefore RS232 settings should be handled in the VFS layer as part of the ioctl core. Regardless, according to Aaron the ZP power-off stuff is currently implemented only in ACPI, tied more closely to the ATA layer than the SCSI layer (though not part of either). It is not part of the SCSI spec in any form. Now it's true that determining whether a device is in the right state for power to be removed involves sending a TEST UNIT READY command, which is of course a SCSI command. This seems to be incidental to the overall scheme, however. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 2/6] scsi: sr: support runtime pm
On 09/30/2012 10:47 AM, Alan Stern wrote: On Sun, 30 Sep 2012, Aaron Lu wrote: Makes sense to me, but there is a problem if I want to block events checking for the disk, as I do not have a pointer to the gendisk in ATA layer. You may discover the gendisk by going the ATA -> SCSI -> block route. The tray will be ejected by the ODD itself when it has power, I do not need to do that. Moreover, I don't think I need enable the GPE bit when it has power. It sounds like you need to add only two things to the sr layer: An interface to enable/disable event checking and an interface to request an eject. (And perhaps ejects can be carried out entirely within the ATAPI layer, with no need to involve sr.) Sounds reasonable. Jeff -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 2/6] scsi: sr: support runtime pm
On 09/29/2012 06:44 PM, Rafael J. Wysocki wrote: On Saturday, September 29, 2012, Aaron Lu wrote: On 09/29/2012 10:29 PM, Alan Stern wrote: On Sat, 29 Sep 2012, Aaron Lu wrote: I don't think this is a good idea, quite frankly. sr seems to be a too generic place for that. Does this mean sr can only have code that is useful to all devices it manages? i.e. If a piece of code enables a feature for a special kind of ODD(like the sata based ZPODD), it shouldn't be done in sr? Drivers are allowed to have special features and quirks that apply only to some devices. I think SATA based zero power capable ODDs are the "some devices". There is nothing in theory stopping us from doing this in ata layer. For the loading mechanism, we can always send an ATAPI command to figure it out. So gentlemen, I need your opinions on where this ZPODD code should live before I can continue this work, thanks. Can arbitrary SCSI devices be ZP, or does this notion apply only to ATAPI-based drives? That's the key question, and the answer determines where the ZP support belongs. I don't know if arbitrary SCSI devices can be ZP or not, the SPC spec doesn't seem to define such a power state. ZPODD is defined for sata based ATAPI ODD which supports device attention, but doesn't specify how the ODD is powered off and how the device attention pin notifies OS. On x86 systems, these are implemented by ACPI. Though SCSI devices may not have a general notion of ZP, the fact that ZPODD are managed by sr driver is enough to make the decision that ZPODD code can live in sr? Well, only a part of it is handled by sr, the high-level part so to speak. The low-level handling is done by the ATA layer. Now, since sr is the high-level part and is supposed to be generic, I don't think it's appropriate to make it care about some low-level things specific to ATA (because there is another layer of code supposed to handle those). On the other hand, the sr driver certainly deserves to have some form of runtime PM support, even for devices that aren't ZP. Agree. And the following codes need to find a home: - Code used to handle ACPI wake notification(currently done in ATA, but causes the "annoying" need_eject flag in scsi_device); And quite frankly I'd more and more convinced that this flag isn't really necessary. What you really want to achieve is to eject the tray of a tray-type ODD if the eject is signaled through a GPE. I don't see anything for sr to do in that. Moreover, you probably would like to do that even if the drive is not powered down, right? I wonder if this mechanism can be used for user space notification without polling regardless of whether or not the zero-power feature is used? Fair questions, and I think this is finally getting to the heart of the matter/solution. - Code to check if the ODD is ready to be powered off per the ZPODD spec. If that can be done at the ATA level, I'd prefer it too. Does the ready-to-poweroff check involve SCSI/MMC command set commands? If no, it probably belongs in libata. If yes, it probably belongs in sr. Jeff -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 2/6] scsi: sr: support runtime pm
On 09/29/2012 06:31 PM, Rafael J. Wysocki wrote: On Saturday, September 29, 2012, Alan Stern wrote: Can arbitrary SCSI devices be ZP, or does this notion apply only to ATAPI-based drives? That's the key question, and the answer determines where the ZP support belongs. I agree. That said for now I'm not aware of any other ZP devices. It also is unclear whether or not their requirements would be the same for the ZPODD devices. Not quite. The key question is whether or not this operates at the SCSI command set level. ATAPI is simply SCSI MMC command set tunnelling. The ATA-specific bits that belong in libata include everything below the SCSI command set: bus details, delivering the command to the device, returning the command response, etc. sr handles the SCSI command set details. SATA optical devices are aligned with the SCSI MMC command set, which periodically synchronizes with USB and ATAPI industry efforts. There are ugly hacks around the edges, where sometimes ATA or USB subsystems may tweak the request or response in passing, but that is the general model: it belongs in libata UNLESS the operation is occurring wholly at the SCSI command set level. Because USB and ATA chose to use the SCSI command set, it is sadly inevitable that there might be a few details -- hopefully glossed over with layer-hopping hooks and flags -- within 'sr' that are bus-specific. The simple fact of "only ZPODD devices out there are ATA" is not the decision-maker for where the code should live. It is more a question where ZPODD belongs in the device/command set model currently employed. Jeff -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH v2 3/5] libfcoe, fcoe, bnx2fc: Add new fcoe control interface
On 09/27/12 04:01, Robert Love wrote:
+static ssize_t store_ctlr_enabled(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct fcoe_ctlr_device *ctlr = dev_to_ctlr(dev);
+ int val;
+ int rc;
+
+ rc = sscanf(buf, "%d", &val);
+ if (!rc)
+ return -EINVAL;
sscanf() expects a '\0'-terminated buffer which is not guaranteed by the
caller of this function (sysfs), isn't it ?
@@ -830,6 +983,18 @@ int __init fcoe_sysfs_setup(void)
if (error)
return error;
+ error = bus_create_file(&fcoe_bus_type, &bus_attr_ctlr_create);
+ if (error) {
+ bus_unregister(&fcoe_bus_type);
+ return error;
+ }
+
+ error = bus_create_file(&fcoe_bus_type, &bus_attr_ctlr_destroy);
+ if (error) {
+ bus_unregister(&fcoe_bus_type);
+ return error;
+ }
+
return 0;
}
It might be a good idea to use fcoe_bus_type.bus_attrs instead of
bus_create_file(). If someone ever would want to trigger these
attributes from udev immediately after an fcoe bus instance has been
created then that approach will avoid a race where udev gets notified
before these attributes got created.
static int fcoe_add_netdev_mapping(struct net_device *netdev,
- struct fcoe_transport *ft)
+ struct fcoe_transport *ft)
Is the above whitespace change necessary ?
Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 2/6] scsi: sr: support runtime pm
On 09/30/2012 10:47 PM, Alan Stern wrote: > On Sun, 30 Sep 2012, Aaron Lu wrote: > >> Makes sense to me, but there is a problem if I want to block events >> checking for the disk, as I do not have a pointer to the gendisk in ATA >> layer. > >> The tray will be ejected by the ODD itself when it has power, I do not >> need to do that. Moreover, I don't think I need enable the GPE bit when >> it has power. > > It sounds like you need to add only two things to the sr layer: An > interface to enable/disable event checking and an interface to request > an eject. (And perhaps ejects can be carried out entirely within the > ATAPI layer, with no need to involve sr.) Thanks for the suggestion, I'll try to do this. -Aaron -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 2/6] scsi: sr: support runtime pm
On Sun, 30 Sep 2012, Aaron Lu wrote: > Makes sense to me, but there is a problem if I want to block events > checking for the disk, as I do not have a pointer to the gendisk in ATA > layer. > The tray will be ejected by the ODD itself when it has power, I do not > need to do that. Moreover, I don't think I need enable the GPE bit when > it has power. It sounds like you need to add only two things to the sr layer: An interface to enable/disable event checking and an interface to request an eject. (And perhaps ejects can be carried out entirely within the ATAPI layer, with no need to involve sr.) Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 2/6] scsi: sr: support runtime pm
On Sun, Sep 30, 2012 at 12:27:41AM +0200, Rafael J. Wysocki wrote: > On Saturday, September 29, 2012, Aaron Lu wrote: > > [Adding more people and list back in] > > > > On 09/29/2012 05:46 AM, Rafael J. Wysocki wrote: > > > On Friday, September 28, 2012, Aaron Lu wrote: > > >> On 09/28/2012 07:15 AM, Rafael J. Wysocki wrote: > > >>> On Thursday, September 27, 2012, Aaron Lu wrote: > > On 09/27/2012 05:37 AM, Rafael J. Wysocki wrote: > > > > > > Say the user has pressed the eject button. What does need to happen > > > so that > > > the tray is physically ejected? > > > > The tray is ejected by the ODD itself, host does not have to do > > anything. > > > > There is a command(PREVENT_MEDIUM_REMOVAL) to lock the door so that > > when > > user presses the eject button, the tray will not be ejected. This > > command > > is usually sent when we have a disc inside and a user space program > > opened the underlying block device(e.g. /dev/sr0) to read/write data. > > > > And host can also eject the tray by sending a START_STOP_UNIT command > > with param LoEj set to 1 and we have a function called sr_tray_move to > > do just this. And this is also what I've used to eject the tray after > > user wakes up the ODD, as when user presses the eject button when the > > ODD is in zero power state, it can't eject the tray as usual, so host > > software will need to do this, that's the reason I need to know such > > information: > > When ODD is resumed, is it because user wakes it up? > > >>> > > >>> But START_STOP_UNIT eventually causes ata_scsi_start_stop_xlat() to be > > >> > > >> You are following ata case, while the ODD is an atapi device :-) > > >> The translation function is atapi_xlat, but that doesn't affect the idea > > >> here. > > >> > > >>> executed, so I wonder if we really need to go up through the SCSI stack > > >>> to send that command to the drive from there? It should be possible > > >>> to issue STANDBY/READ VERIFY to the device directly from libata if > > >>> an eject event is signaled through a GPE. > > >> > > >> Yes, this is possible. > > >> Though it doesn't feel very cool, since I have no idea if the ODD is a > > >> tray type or slot type in ATA layer and I'll blindly send this command > > >> to it then, not a problem maybe. > > > > > > It would be good to verify if that works for slot devices, if possible. > > > > The ACPI GPE event is triggered when user inserts a disc into a slot > > type ODD, and if I send an eject command to it, the disc will be > > ejected, which is wrong. > > > > I need to know the loading mechanism(tray type or slot type) of the ODD > > to decide if I should send this command. > > > > > > > >> And what do you think of moving the acpi notification code to sr? > > >> http://marc.info/?l=linux-pm&m=134873904332704&w=4 > > > > > > I don't think this is a good idea, quite frankly. sr seems to be a too > > > generic place for that. > > > > Does this mean sr can only have code that is useful to all devices it > > manages? i.e. If a piece of code enables a feature for a special kind of > > ODD(like the sata based ZPODD), it shouldn't be done in sr? > > If the feature is specific to one special kind of ODD only, then I don't > think sr is the right place to add support for it. > > > > Ideally, the whole ZPODD handling should not be visible to the SCSI layer, > > > > I can see 2 problems: > > - Don't know its loading machanism so we have the problem above; > > Does using the need_eject flag address this problem somehow? The need_eject flag is used to give sr a hint that on resume, please eject its tray. And sr knows what loading mechanism this ODD is. > > > - Need to send command to find out if ODD is zero power ready somewhere > > in ata layer, this implies the device is doing IO after it is runtime > > suspended in scsi layer. > > There's nothing wrong with accessig suspended devices as long as we know > that they will respond. :-) Oh, I was reading the comments above the struct dev_pm_ops in /include/linux/pm.h and thought that was a requirement :-) Thanks, Aaron > > > > perhaps except the "no_polling" flag disabling the polling that may be > > > useful for other purposes in principle. > > > > I hope so, let's hear what other people has to say. > > > > > > > > I'm not sure if it's possible at this point, but if not we need to know > > > very precisely why not. > > > > There is nothing in theory stopping us from doing this in ata layer. > > For the loading mechanism, we can always send an ATAPI command to figure > > it out. > > > > So gentlemen, I need your opinions on where this ZPODD code should live > > before I can continue this work, thanks. > > I would _try_ to add it at the ATA level. > > Thanks, > Rafael > -- > To unsubscribe from this list: send the line "unsubscribe linux-ide" in > the body of a message to majord...@vger.ker
Re: [PATCH v7 2/6] scsi: sr: support runtime pm
On Sun, Sep 30, 2012 at 12:44:50AM +0200, Rafael J. Wysocki wrote: > On Saturday, September 29, 2012, Aaron Lu wrote: > > On 09/29/2012 10:29 PM, Alan Stern wrote: > > > On Sat, 29 Sep 2012, Aaron Lu wrote: > > > > > >>> I don't think this is a good idea, quite frankly. sr seems to be a too > > >>> generic place for that. > > >> > > >> Does this mean sr can only have code that is useful to all devices it > > >> manages? i.e. If a piece of code enables a feature for a special kind of > > >> ODD(like the sata based ZPODD), it shouldn't be done in sr? > > > > > > Drivers are allowed to have special features and quirks that apply only > > > to some devices. > > > > I think SATA based zero power capable ODDs are the "some devices". > > > > > > > >> There is nothing in theory stopping us from doing this in ata layer. > > >> For the loading mechanism, we can always send an ATAPI command to figure > > >> it out. > > >> > > >> So gentlemen, I need your opinions on where this ZPODD code should live > > >> before I can continue this work, thanks. > > > > > > Can arbitrary SCSI devices be ZP, or does this notion apply only to > > > ATAPI-based drives? That's the key question, and the answer determines > > > where the ZP support belongs. > > > > I don't know if arbitrary SCSI devices can be ZP or not, the SPC spec > > doesn't seem to define such a power state. > > > > ZPODD is defined for sata based ATAPI ODD which supports device > > attention, but doesn't specify how the ODD is powered off and how the > > device attention pin notifies OS. On x86 systems, these are implemented > > by ACPI. > > > > Though SCSI devices may not have a general notion of ZP, the fact that > > ZPODD are managed by sr driver is enough to make the decision that ZPODD > > code can live in sr? > > Well, only a part of it is handled by sr, the high-level part so to speak. > The low-level handling is done by the ATA layer. Now, since sr is the > high-level part and is supposed to be generic, I don't think it's appropriate > to make it care about some low-level things specific to ATA (because there is > another layer of code supposed to handle those). Makes sense to me, but there is a problem if I want to block events checking for the disk, as I do not have a pointer to the gendisk in ATA layer. > > > > On the other hand, the sr driver certainly deserves to have some form > > > of runtime PM support, even for devices that aren't ZP. > > > > Agree. > > > > And the following codes need to find a home: > > - Code used to handle ACPI wake notification(currently done in ATA, but > > causes the "annoying" need_eject flag in scsi_device); > > And quite frankly I'd more and more convinced that this flag isn't really > necessary. > > What you really want to achieve is to eject the tray of a tray-type ODD > if the eject is signaled through a GPE. I don't see anything for sr to > do in that. Moreover, you probably would like to do that even if the > drive is not powered down, right? The tray will be ejected by the ODD itself when it has power, I do not need to do that. Moreover, I don't think I need enable the GPE bit when it has power. > > I wonder if this mechanism can be used for user space notification > without polling regardless of whether or not the zero-power feature is > used? This may be a reason the GPE should be always enabled no matter if power is removed or not. But I have concerns that this mechanism is designed to acheive this, as explained in another mail thread: http://marc.info/?l=linux-pm&m=134882049212936&w=2 Copied here: The SATA spec says the device attention pin shall assert when: - For tray type ODD, its front panel button is released; - For slot type ODD, media is inserted. I've a slot type ODD which has a eject button. The notification will be sent when a disc is inserted, but not when the eject button is pressed, and this doesn't violate the spec. But if we disable the poll for disc changes, we will lose an event when the disc is ejected by the eject button(the device attention pin shall not trigger this time). I suppose this is a problem? I think the device attention scheme is not designed to do this job, while SATA asynchronous notification is. Thanks, Aaron > > > - Code to check if the ODD is ready to be powered off per the ZPODD > > spec. > > If that can be done at the ATA level, I'd prefer it too. > > Thanks, > Rafael > -- > To unsubscribe from this list: send the line "unsubscribe linux-pm" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] qla2xxx: silence two GCC warnings
Compiling qla_gs.o (part of the qla2xxx module) triggers two GCC warnings: drivers/scsi/qla2xxx/qla_gs.c: In function ‘qla2x00_fdmi_rhba’: drivers/scsi/qla2xxx/qla_gs.c:1339:7: warning: array subscript is above array bounds [-Warray-bounds] drivers/scsi/qla2xxx/qla_gs.c: In function ‘qla2x00_fdmi_register’: drivers/scsi/qla2xxx/qla_gs.c:1663:15: warning: array subscript is above array bounds [-Warray-bounds] It seems that the sequence of a strcpy followed by a strlen confuses GCC when it is keeping track of array bounds here. (It is not clear to me which array triggers this warning and by how much GCC thinks the subscript is above its bounds. Neither is it clear to me why comparable code in these two functions doesn't trigger this warning.) The easiest way to silence these warnings is to hardcode the length of these two strings in the code here. The length used here is the length of the string, including its NUL terminator, rounded up to the next multiple of four. Signed-off-by: Paul Bolle --- 0) I noticed this warning while building v3.6-rc7 on current Fedora 17, using Fedora's default config. 1) Compile tested only. drivers/scsi/qla2xxx/qla_gs.c | 12 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/drivers/scsi/qla2xxx/qla_gs.c b/drivers/scsi/qla2xxx/qla_gs.c index 05260d2..a3ef5d0 100644 --- a/drivers/scsi/qla2xxx/qla_gs.c +++ b/drivers/scsi/qla2xxx/qla_gs.c @@ -1326,10 +1326,8 @@ qla2x00_fdmi_rhba(scsi_qla_host_t *vha) eiter = (struct ct_fdmi_hba_attr *) (entries + size); eiter->type = __constant_cpu_to_be16(FDMI_HBA_MANUFACTURER); strcpy(eiter->a.manufacturer, "QLogic Corporation"); - alen = strlen(eiter->a.manufacturer); - alen += (alen & 3) ? (4 - (alen & 3)) : 4; - eiter->len = cpu_to_be16(4 + alen); - size += 4 + alen; + eiter->len = cpu_to_be16(4 + 20); + size += 4 + 20; ql_dbg(ql_dbg_disc, vha, 0x2026, "Manufacturer = %s.\n", eiter->a.manufacturer); @@ -1647,10 +1645,8 @@ qla2x00_fdmi_rpa(scsi_qla_host_t *vha) eiter = (struct ct_fdmi_port_attr *) (entries + size); eiter->type = __constant_cpu_to_be16(FDMI_PORT_OS_DEVICE_NAME); strcpy(eiter->a.os_dev_name, QLA2XXX_DRIVER_NAME); - alen = strlen(eiter->a.os_dev_name); - alen += (alen & 3) ? (4 - (alen & 3)) : 4; - eiter->len = cpu_to_be16(4 + alen); - size += 4 + alen; + eiter->len = cpu_to_be16(4 + 8); + size += 4 + 8; ql_dbg(ql_dbg_disc, vha, 0x204b, "OS_Device_Name=%s.\n", eiter->a.os_dev_name); -- 1.7.11.4 -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
