- 23 Jan, 2021 40 commits
-
-
Ahmed S. Darwish authored
mvsas calls the non _gfp version of the libsas event notifiers API, leading to the buggy call chains below: mvsas/mv_sas.c: mvs_work_queue() [process context] spin_lock_irqsave(mvs_info::lock, ) -> libsas/sas_event.c: sas_notify_phy_event() -> sas_alloc_event() -> in_interrupt() = false -> invalid GFP_KERNEL allocation -> libsas/sas_event.c: sas_notify_port_event() -> sas_alloc_event() -> in_interrupt() = false -> invalid GFP_KERNEL allocation Use the new event notifiers API instead, which requires callers to explicitly pass the gfp_t memory allocation flags. Below are context analysis for the modified functions: => mvs_bytes_dmaed(): Since it is invoked from both process and atomic contexts, let its callers pass the gfp_t flags. Call chains: scsi_scan.c: do_scsi_scan_host() [has msleep()] -> shost->hostt->scan_start() -> [mvsas/mv_init.c: Scsi_Host::scsi_host_template .scan_start = mvs_scan_start()] -> mvsas/mv_sas.c: mvs_scan_start() -> mvs_bytes_dmaed(..., GFP_KERNEL) mvsas/mv_sas.c: mvs_work_queue() spin_lock_irqsave(mvs_info::lock,) -> mvs_bytes_dmaed(..., GFP_ATOMIC) mvsas/mv_64xx.c: mvs_64xx_isr() || mvsas/mv_94xx.c: mvs_94xx_isr() -> mvsas/mv_chips.h: mvs_int_full() -> mvsas/mv_sas.c: mvs_int_port() -> mvs_bytes_dmaed(..., GFP_ATOMIC); => mvs_work_queue(): Invoked from process context, but it calls all the libsas event notifier APIs under a spin_lock_irqsave(). Pass GFP_ATOMIC. Link: https://lore.kernel.org/r/20210118100955.1761652-5-a.darwish@linutronix.de Fixes: 1c393b97 ("scsi: libsas: Use dynamic alloced work to avoid sas event lost") Cc: Jason Yan <yanaijie@huawei.com> Reviewed-by:John Garry <john.garry@huawei.com> Signed-off-by:
Ahmed S. Darwish <a.darwish@linutronix.de> Signed-off-by:
Martin K. Petersen <martin.petersen@oracle.com>
-
Ahmed S. Darwish authored
sas_alloc_event() uses in_interrupt() to decide which allocation should be used. The usage of in_interrupt() in drivers is phased out and Linus clearly requested that code which changes behaviour depending on context should either be separated or the context be conveyed in an argument passed by the caller, which usually knows the context. The in_interrupt() check is also only partially correct, because it fails to choose the correct code path when just preemption or interrupts are disabled. For example, as in the following call chain: mvsas/mv_sas.c: mvs_work_queue() [process context] spin_lock_irqsave(mvs_info::lock, ) -> libsas/sas_event.c: sas_notify_phy_event() -> sas_alloc_event() -> in_interrupt() = false -> invalid GFP_KERNEL allocation -> libsas/sas_event.c: sas_notify_port_event() -> sas_alloc_event() -> in_interrupt() = false -> invalid GFP_KERNEL allocation Introduce sas_alloc_event_gfp(), sas_notify_port_event_gfp(), and sas_notify_phy_event_gfp(), which all behave like the non _gfp() variants but use a caller-passed GFP mask for allocations. For bisectability, all callers will be modified first to pass GFP context, then the non _gfp() libsas API variants will be modified to take a gfp_t by default. Link: https://lore.kernel.org/r/20210118100955.1761652-4-a.darwish@linutronix.de Fixes: 1c393b97 ("scsi: libsas: Use dynamic alloced work to avoid sas event lost") Cc: Jason Yan <yanaijie@huawei.com> Reviewed-by: -
John Garry authored
LLDDs report events to libsas with .notify_port_event and .notify_phy_event callbacks. These callbacks are fixed and so there is no reason why the functions cannot be called directly, so do that. This neatens the code slightly, makes it more obvious, and reduces function pointer usage, which is generally a good thing. Downside is that there are 2x more symbol exports. [a.darwish@linutronix.de: Remove the now unused "sas_ha" local variables] Link: https://lore.kernel.org/r/20210118100955.1761652-3-a.darwish@linutronix.deReviewed-by:
Christoph Hellwig <hch@lst.de> Reviewed-by:
Jack Wang <jinpu.wang@cloud.ionos.com> Signed-off-by:
-
Ahmed S. Darwish authored
The ->notify_ha_event() hook has long been removed from the libsas event interface. Remove it from documentation. Link: https://lore.kernel.org/r/20210118100955.1761652-2-a.darwish@linutronix.de Fixes: 042ebd29 ("scsi: libsas: kill useless ha_event and do some cleanup") Cc: stable@vger.kernel.org Reviewed-by:
-
Hannes Reinecke authored
Use SAM status values instead of the driver-defined ones. This also fixes a potential bug as the driver-defined values declare 'COMMAND TERMINATED' with a value of 0x20, whereas SCSI-II defines it with a value of 0x22. Link: https://lore.kernel.org/r/20210113090500.129644-36-hare@suse.deReviewed-by:
Hannes Reinecke <hare@suse.de> Signed-off-by:
-
Hannes Reinecke authored
Replace the driver-defined status byte accessors with the mid-layer defined ones. Link: https://lore.kernel.org/r/20210113090500.129644-35-hare@suse.deReviewed-by:
-
Hannes Reinecke authored
fc_remote_port_chkready() returns a SCSI result value, not the port status. Fix the value returned when the remote port isn't set. Link: https://lore.kernel.org/r/20210113090500.129644-34-hare@suse.deReviewed-by:
Himanshu Madhani <himanshu.madhani@oracle.com> Signed-off-by:
-
Hannes Reinecke authored
ILLEGAL_COMMAND is a sense code, not a driver byte. Link: https://lore.kernel.org/r/20210113090500.129644-33-hare@suse.deReviewed-by:
-
Hannes Reinecke authored
A non-zero queuecommand() return code means 'busy', i.e. the command hasn't been submitted. So any command which should be failed need to be completed via the ->scsi_done() callback with the appropriate result code set. Link: https://lore.kernel.org/r/20210113090500.129644-32-hare@suse.deReviewed-by:
-
Hannes Reinecke authored
Use standard SCSI status and drop usage of the linux-specific ones. Link: https://lore.kernel.org/r/20210113090500.129644-31-hare@suse.deReviewed-by:
Bart Van Assche <bvanassche@acm.org> Reviewed-by:
-
Hannes Reinecke authored
The message byte setting always devolves to COMMAND_COMPLETE so we can drop setting the message byte in the SCSI result. Link: https://lore.kernel.org/r/20210113090500.129644-30-hare@suse.deReviewed-by:
-
Hannes Reinecke authored
Just pass in the host byte to esp_cmd_is_done() and set the status or message bytes if the host byte is DID_OK. Link: https://lore.kernel.org/r/20210113090500.129644-29-hare@suse.deAcked-by:
Finn Thain <fthain@telegraphics.com.au> Signed-off-by:
-
Hannes Reinecke authored
Add the missing 'set_status_byte()' accessor function. Link: https://lore.kernel.org/r/20210113090500.129644-28-hare@suse.deReviewed-by:
Benjamin Block <bblock@linux.ibm.com> Signed-off-by:
-
Hannes Reinecke authored
Change the error code for an invalid SCSI opcode to DID_ERROR. INITIATOR_ERROR is a scsi parallel message which doesn't apply for RAID HBAs. Link: https://lore.kernel.org/r/20210113090500.129644-27-hare@suse.deReviewed-by:
-
Hannes Reinecke authored
CMD_ACCEPT_MSG is an internal definition and most certainly not a SCSI status. As the latter gets set during command completion we can drop the assignment here. Link: https://lore.kernel.org/r/20210113090500.129644-26-hare@suse.deReviewed-by:
-
Hannes Reinecke authored
Use standard definitions for SCSI commands and return status instead of the hardcoded values. Link: https://lore.kernel.org/r/20210113090500.129644-25-hare@suse.deReviewed-by:
-
Hannes Reinecke authored
COMMAND_COMPLETE is defined as '0', so setting it is quite pointless. Link: https://lore.kernel.org/r/20210113090500.129644-24-hare@suse.deReviewed-by:
-
Hannes Reinecke authored
COMMAND_COMPLETE is defined as '0', so setting it is quite pointless. Link: https://lore.kernel.org/r/20210113090500.129644-23-hare@suse.deReviewed-by:
Douglas Gilbert <dgilbert@interlog.com> Signed-off-by:
-
Hannes Reinecke authored
Use the standard SCSI message definitions instead of the driver-internal ones. Link: https://lore.kernel.org/r/20210113090500.129644-22-hare@suse.deReviewed-by:
-
Hannes Reinecke authored
Drop the internal SCSI message definitions and use the functions provided by the SPI transport class. Link: https://lore.kernel.org/r/20210113090500.129644-21-hare@suse.deReported-by:
kernel test robot <lkp@intel.com> Reviewed-by:
-
Hannes Reinecke authored
Use the standard SCSI message definitions instead of the driver-internal ones. Link: https://lore.kernel.org/r/20210113090500.129644-20-hare@suse.deReviewed-by:
-
Hannes Reinecke authored
Use the standard SCSI message definitions instead of the driver-internal ones. Link: https://lore.kernel.org/r/20210113090500.129644-19-hare@suse.deReviewed-by:
-
Hannes Reinecke authored
COMMAND_COMPLETE is defined as '0', so setting it is quite pointless. Link: https://lore.kernel.org/r/20210113090500.129644-18-hare@suse.deReviewed-by:
-
Hannes Reinecke authored
COMMAND_COMPLETE is defined as '0', and it is a SCSI parallel message to boot. Drop the call to set_msg_byte(). Link: https://lore.kernel.org/r/20210113090500.129644-17-hare@suse.deReviewed-by:
Don Brace <don.brace@microchip.com> Signed-off-by:
-
Hannes Reinecke authored
The aacraid controller is a RAID controller and the driver will never see any SCSI messages. Plus it's quite pointless to set the message byte if the host byte is already set, as the latter takes precedence during error recovery. Drop the message byte values for the final result. Link: https://lore.kernel.org/r/20210113090500.129644-16-hare@suse.deReviewed-by:
-
Hannes Reinecke authored
COMMAND_COMPLETE is defined as '0', and it is a SCSI parallel message to boot. So drop the call to set_msg_byte(). Link: https://lore.kernel.org/r/20210113090500.129644-15-hare@suse.deReviewed-by:
-
Hannes Reinecke authored
Use standard SAM status definitions and drop the driver-defined ones. Link: https://lore.kernel.org/r/20210113090500.129644-14-hare@suse.deReviewed-by:
-
Hannes Reinecke authored
We don't need to duplicate definitions from the common include files. Link: https://lore.kernel.org/r/20210113090500.129644-13-hare@suse.deReviewed-by:
-
Hannes Reinecke authored
SCp.status is always the SAM-defined status value, not the Linux ones. Fixup the one wrong definition. Link: https://lore.kernel.org/r/20210113090500.129644-12-hare@suse.deReviewed-by:
-
Hannes Reinecke authored
Use midlayer-defined values and drop the non-existing QUEUE_FULL case; we are checking the SCSI messages in the switch statement, and QUEUE_FULL is a SCSI status hence it can never occur here. Link: https://lore.kernel.org/r/20210113090500.129644-11-hare@suse.deReviewed-by:
-
Hannes Reinecke authored
Drop the driver-defined SCSI status codes and use the generic ones instead. Link: https://lore.kernel.org/r/20210113090500.129644-10-hare@suse.deReviewed-by:
-
Hannes Reinecke authored
Replace the driver-defined SAM status definitions with the standard mid-layer defined ones. Link: https://lore.kernel.org/r/20210113090500.129644-9-hare@suse.deReviewed-by:
-
Hannes Reinecke authored
Link: https://lore.kernel.org/r/20210113090500.129644-8-hare@suse.deReviewed-by:
-
Hannes Reinecke authored
Link: https://lore.kernel.org/r/20210113090500.129644-7-hare@suse.deSigned-off-by:
-
Hannes Reinecke authored
Link: https://lore.kernel.org/r/20210113090500.129644-6-hare@suse.deSigned-off-by:
-
Hannes Reinecke authored
Link: https://lore.kernel.org/r/20210113090500.129644-5-hare@suse.deSigned-off-by:
-
Hannes Reinecke authored
Link: https://lore.kernel.org/r/20210113090500.129644-4-hare@suse.deSigned-off-by:
-
Hannes Reinecke authored
Link: https://lore.kernel.org/r/20210113090500.129644-3-hare@suse.deSigned-off-by:
-
Hannes Reinecke authored
The gdth driver refers to a SCSI parallel, PCI-only HBA RAID adapter which was manufactured by the now-defunct ICP Vortex company, later acquired by Adaptec and superseded by the aacraid series of controllers. The driver itself would require a major overhaul before any modifications can be attempted, but seeing that it's unlikely to have any users left it should rather be removed completely. Link: https://lore.kernel.org/r/20210113090500.129644-2-hare@suse.deCautiously-Acked-by:
-
Ahmed S. Darwish authored
transport_handle_cdb_direct() uses in_interrupt() to detect if it is safe to sleep. It produces a stack trace and returns with an error which is clearly for debugging. The usage of in_interrupt() in drivers is phased out and Linus clearly requested that code which changes behaviour depending on context should either be separated or the context be conveyed in an argument passed by the caller, which usually knows the context. transport_handle_cdb_direct() has a comment saying that it may only be invoked from process context. It invokes transport_generic_new_cmd() which performs GFP_KERNEL memory allocations. in_interrupt() does not detect all the contexts where it is invalid to sleep (for the blocking GFP_KERNEL allocation) as it fails to detect sections with disabled preemption. Replace the in_interrupt() based check with a might_sleep() annotation. Link: https://lore.kernel.org/r/20201220203638.43615-7-bigeasy@linutronix.deSigned-off-by:
Sebastian Andrzej Siewior <bigeasy@linutronix.de> Signed-off-by:
-
