scsi: leapraid: Rename leapioraid to leapraid and release v2.0#1467
Conversation
The LeapRAID driver provides support for LeapRAID PCIe RAID controllers, enabling communication between the host operating system, firmware, and hardware for efficient storage management. The main source files are organized as follows: leapraid_os.c: Implements the scsi_host_template functions, PCIe device probing, and initialization routines, integrating the driver with the Linux SCSI subsystem. leapraid_func.c: Provides the core functional routines that handle low-level interactions with the controller firmware and hardware, including interrupt handling, topology management, and reset sequence processing, and other related operations. leapraid_app.c: Implements the ioctl interface, providing user-space tools access to device management and diagnostic operations. leapraid_transport.c: Interacts with the Linux SCSI transport layer to add SAS phys and ports. leapraid_func.h: Declares common data structures, constants, and function prototypes shared across the driver. leapraid.h: Provides global constants, register mappings, and interface definitions that facilitate communication between the driver and the controller firmware. The leapraid_probe function is called when the driver detects a supported LeapRAID PCIe device. It allocates and initializes the Scsi_Host structure, configures hardware and firmware interfaces, and registers the host adapter with the Linux SCSI mid-layer. After registration, the driver invokes scsi_scan_host() to initiate device discovery. The firmware then reports discovered logical and physical devices to the host through interrupt-driven events and synchronizes their operational states. leapraid_adapter is the core data structure that encapsulates all resources and runtime state information maintained during driver operation, described as follows: /** * struct leapraid_adapter - Main LeapRaid adapter structure * @list: List head for adapter management * @shost: SCSI host structure * @pdev: PCI device structure * @iomem_base: I/O memory mapped base address * @rep_msg_host_idx: Host index for reply messages * @mask_int: Interrupt masking flag * @timestamp_sync_cnt: Timestamp synchronization counter * @adapter_attr: Adapter attributes * @mem_desc: Memory descriptor * @driver_cmds: Driver commands * @dynamic_task_desc: Dynamic task descriptor * @fw_evt_s: Firmware event structure * @notification_desc: Notification descriptor * @reset_desc: Reset descriptor * @scan_dev_desc: Device scan descriptor * @access_ctrl: Access control * @fw_log_desc: Firmware log descriptor * @dev_topo: Device topology * @boot_devs: Boot devices * @smart_poll_desc: SMART polling descriptor */ struct leapraid_adapter { struct list_head list; struct Scsi_Host *shost; struct pci_dev *pdev; struct leapraid_reg_base __iomem *iomem_base; u32 rep_msg_host_idx; bool mask_int; u32 timestamp_sync_cnt; struct leapraid_adapter_attr adapter_attr; struct leapraid_mem_desc mem_desc; struct leapraid_driver_cmds driver_cmds; struct leapraid_dynamic_task_desc dynamic_task_desc; struct leapraid_fw_evt_struct fw_evt_s; struct leapraid_notification_desc notification_desc; struct leapraid_reset_desc reset_desc; struct leapraid_scan_dev_desc scan_dev_desc; struct leapraid_access_ctrl access_ctrl; struct leapraid_fw_log_desc fw_log_desc; struct leapraid_dev_topo dev_topo; struct leapraid_boot_devs boot_devs; struct leapraid_smart_poll_desc smart_poll_desc; }; Signed-off-by: Hao Dongdong <doubled@leap-io-kernel.com>
There was a problem hiding this comment.
Sorry @leap-io, your pull request is larger than the review limit of 150000 diff characters
There was a problem hiding this comment.
Pull request overview
This PR renames the leapioraid driver to leapraid and releases version 2.0. The changes include refactoring RAID-related logic, streamlining firmware event handling, simplifying drive removal paths, updating coding style to comply with Linux kernel guidelines, adding 16 debug logging registers, and introducing a dedicated logging subsystem.
Changes:
- Complete driver rename from
leapioraidtoleapraid - New driver implementation files (leapraid_transport.c, leapraid_func.h, leapraid_app.c)
- Updated build system (Makefile, Kconfig) and configuration files
- Removed old leapioraid driver files
Reviewed changes
Copilot reviewed 18 out of 21 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| drivers/scsi/leapraid/leapraid_transport.c | New transport layer implementation with SAS device management |
| drivers/scsi/leapraid/leapraid_func.h | New header with data structures and function declarations |
| drivers/scsi/leapraid/leapraid_app.c | New application interface for ioctl operations |
| drivers/scsi/leapraid/Makefile | Build configuration for new driver |
| drivers/scsi/leapraid/Kconfig | Kernel configuration for new driver |
| drivers/scsi/Makefile | Updated to reference leapraid instead of leapioraid |
| drivers/scsi/Kconfig | Updated driver source path |
| arch//configs/_defconfig | Updated configuration symbol names |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ssize_t __maybe_unused ret; | ||
|
|
||
| smp_passthrough_rep = | ||
| (void *)(&adapter->driver_cmds.transport_cmd.reply); | ||
| if (le16_to_cpu(smp_passthrough_rep->resp_data_len) != | ||
| sizeof(struct leapraid_rep_manu_reply)) | ||
| return; | ||
|
|
||
| rep_manu_reply = data_out + sizeof(struct leapraid_rep_manu_request); | ||
| ret = strscpy(edev->vendor_id, rep_manu_reply->vendor_identification, | ||
| SAS_EXPANDER_VENDOR_ID_LEN); | ||
| ret = strscpy(edev->product_id, rep_manu_reply->product_identification, | ||
| SAS_EXPANDER_PRODUCT_ID_LEN); | ||
| ret = strscpy(edev->product_rev, | ||
| rep_manu_reply->product_revision_level, | ||
| SAS_EXPANDER_PRODUCT_REV_LEN); | ||
| edev->level = rep_manu_reply->sas_format & 1; | ||
| if (edev->level) { | ||
| ret = strscpy(edev->component_vendor_id, | ||
| rep_manu_reply->component_vendor_identification, | ||
| SAS_EXPANDER_COMPONENT_VENDOR_ID_LEN); | ||
|
|
||
| component_id = (u8 *)&rep_manu_reply->component_id; | ||
| edev->component_id = component_id[0] << 8 | component_id[1]; | ||
| edev->component_revision_id = | ||
| rep_manu_reply->component_revision_level; | ||
| } |
There was a problem hiding this comment.
The __maybe_unused attribute on the ret variable (line 251) is unnecessary and indicates the return values from strscpy are being ignored. The function should check these return values to ensure the copy operations succeeded and handle truncation appropriately, especially for critical fields like vendor_id and product_id.
| ssize_t __maybe_unused ret; | ||
| u8 revision; | ||
|
|
||
| karg = kzalloc(sizeof(*karg), GFP_KERNEL); | ||
| if (!karg) | ||
| return -ENOMEM; | ||
|
|
||
| pci_read_config_byte(adapter->pdev, PCI_CLASS_REVISION, &revision); | ||
| karg->revision = revision; | ||
| karg->pci_id = adapter->pdev->device; | ||
| karg->sub_dev = adapter->pdev->subsystem_device; | ||
| karg->sub_vendor = adapter->pdev->subsystem_vendor; | ||
| karg->pci_info.u.bits.bus = adapter->pdev->bus->number; | ||
| karg->pci_info.u.bits.dev = PCI_SLOT(adapter->pdev->devfn); | ||
| karg->pci_info.u.bits.func = PCI_FUNC(adapter->pdev->devfn); | ||
| karg->pci_info.seg_id = pci_domain_nr(adapter->pdev->bus); | ||
| karg->fw_ver = adapter->adapter_attr.features.fw_version; | ||
| ret = strscpy(karg->driver_ver, LEAPRAID_DRIVER_NAME, | ||
| sizeof(karg->driver_ver)); | ||
| strcat(karg->driver_ver, "-"); | ||
| strcat(karg->driver_ver, LEAPRAID_DRIVER_VERSION); |
There was a problem hiding this comment.
The __maybe_unused attribute on the ret variable (line 490) is unnecessary and indicates the return value from strscpy is being ignored. The function should check the return value to ensure the copy operation succeeded and handle potential truncation issues.
| @@ -0,0 +1,14 @@ | |||
| # SPDX-License-Identifier: GPL-2.0-or-later | |||
There was a problem hiding this comment.
Missing SPDX license identifier format. The Kconfig file uses "GPL-2.0-or-later" while the C source files use "GPL-2.0". This inconsistency should be resolved to ensure clear licensing. If the driver is GPL-2.0 only, the Kconfig should match.
| # SPDX-License-Identifier: GPL-2.0-or-later | |
| # SPDX-License-Identifier: GPL-2.0 |
| if (card_phy->attached_hdl) { | ||
| if (leapraid_transport_set_identify(adapter, | ||
| card_phy->attached_hdl, | ||
| &card_phy->remote_identify)) { | ||
| dev_err(&adapter->pdev->dev, | ||
| "%s set phy attached handle identify failed!\n", | ||
| __func__); | ||
| sas_phy_free(phy); | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
Missing return statement after error handling. When leapraid_transport_set_identify fails at line 948-950, the function should return instead of continuing to execute line 959 which will attempt to use phy that should have been freed.
| dev_err(&adapter->pdev->dev, | ||
| "%s set phy attached hdl identify failed!\n", | ||
| __func__); | ||
| sas_phy_free(phy); |
There was a problem hiding this comment.
Missing return statement after error handling. When leapraid_transport_set_identify fails at line 994-996, the function should return an error code instead of continuing to execute line 1004 which will attempt to use phy that should have been freed.
| sas_phy_free(phy); | |
| sas_phy_free(phy); | |
| return -EFAULT; |
| if ((sas_phy_add(phy))) { | ||
| sas_phy_free(phy); | ||
| return -EFAULT; | ||
| } |
There was a problem hiding this comment.
Inconsistent error return value. After calling sas_phy_add fails at line 1006, the function returns -EFAULT. However, at line 961 in leapraid_transport_add_card_phy, a similar failure is handled by returning without an error code. Consider standardizing error handling - either both should return an error, or both should return void and handle errors consistently.
| list_for_each_entry_safe(card_phy, card_phy_next, | ||
| &sas_port->phy_list, port_siblings) | ||
| list_del(&card_phy->port_siblings); | ||
| kfree(sas_port); |
There was a problem hiding this comment.
Potential use-after-free vulnerability. The function frees sas_port at line 712 in the error path, but there's no null check before this. If sas_port allocation succeeded but subsequent operations failed, the cleanup at lines 709-711 could access freed memory if any of those list operations fail.
| list_for_each_entry_safe(card_phy, card_phy_next, | |
| &sas_port->phy_list, port_siblings) | |
| list_del(&card_phy->port_siblings); | |
| kfree(sas_port); | |
| if (sas_port) { | |
| list_for_each_entry_safe(card_phy, card_phy_next, | |
| &sas_port->phy_list, port_siblings) | |
| list_del(&card_phy->port_siblings); | |
| kfree(sas_port); | |
| } |
| if (sas_port->remote_identify.device_type == | ||
| LEAPRAID_DEVTYP_EDGE_EXPANDER || | ||
| sas_port->remote_identify.device_type == | ||
| LEAPRAID_DEVTYP_FANOUT_EXPANDER) | ||
| leapraid_transport_exp_report_manu(adapter, | ||
| sas_port->remote_identify.sas_address, | ||
| rphy_to_expander_device(rphy), | ||
| card_port->port_id); |
There was a problem hiding this comment.
Incorrect device type comparison. Lines 698-700 compare sas_port->remote_identify.device_type with LEAPRAID_DEVTYP_EDGE_EXPANDER and LEAPRAID_DEVTYP_FANOUT_EXPANDER, but these constants are likely from the internal device info flags (based on patterns in leapraid_func.h). The device_type field should be compared against SAS standard types like SAS_EDGE_EXPANDER_DEVICE and SAS_FANOUT_EXPANDER_DEVICE (as used correctly in lines 232-235).
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: opsiff The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Refactor RAID-related logic, streamline firmware event handling, and simplify drive removal paths.
Update coding style and structure to comply with Linux kernel coding guidelines.
Add 16 debug logging registers.
Introduce a dedicated logging subsystem.