Re: [PATCH -next] scsi: pm80xx: Remove set but not used variable 'page_code'

[Jinpu Wang]

On Fri, Sep 14, 2018 at 3:29 AM YueHaibing  wrote:
>
> Fixes gcc '-Wunused-but-set-variable' warning:
>
> drivers/scsi/pm8001/pm80xx_hwi.c: In function 'pm8001_set_phy_profile':
> drivers/scsi/pm8001/pm80xx_hwi.c:4679:6: warning:
>  variable 'page_code' set but not used [-Wunused-but-set-variable]
>
> Signed-off-by: YueHaibing 
Acked-by: Jack Wang 
Thanks!
> ---
>  drivers/scsi/pm8001/pm80xx_hwi.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/scsi/pm8001/pm80xx_hwi.c 
> b/drivers/scsi/pm8001/pm80xx_hwi.c
> index b641875..9864a3c 100644
> --- a/drivers/scsi/pm8001/pm80xx_hwi.c
> +++ b/drivers/scsi/pm8001/pm80xx_hwi.c
> @@ -4676,9 +4676,8 @@ void mpi_set_phy_profile_req(struct pm8001_hba_info 
> *pm8001_ha,
>  void pm8001_set_phy_profile(struct pm8001_hba_info *pm8001_ha,
> u32 length, u8 *buf)
>  {
> -   u32 page_code, i;
> +   u32 i;
>
> -   page_code = SAS_PHY_ANALOG_SETTINGS_PAGE;
> for (i = 0; i < pm8001_ha->chip->n_phy; i++) {
> mpi_set_phy_profile_req(pm8001_ha,
> SAS_PHY_ANALOG_SETTINGS_PAGE, i, length, (u32 *)buf);
>


-- 
Jack Wang
Linux Kernel Developer

ProfitBricks GmbH
Greifswalder Str. 207
D - 10405 Berlin

Tel:   +49 30 577 008  042
Fax:  +49 30 577 008 299
Email:jinpu.w...@profitbricks.com
URL:  https://www.profitbricks.de

Sitz der Gesellschaft: Berlin
Registergericht: Amtsgericht Charlottenburg, HRB 125506 B
Geschäftsführer: Achim Weiss, Matthias Steinberg, Christoph Steffens

Re: [PATCH -next] scsi: pm80xx: Remove set but not used variable 'device_id'

[Jinpu Wang]

On Thu, Sep 13, 2018 at 3:44 AM YueHaibing  wrote:
>
> Fixes gcc '-Wunused-but-set-variable' warning:
>
> drivers/scsi/pm8001/pm8001_sas.c: In function 
> 'pm8001_I_T_nexus_event_handler':
> drivers/scsi/pm8001/pm8001_sas.c:1052:6: warning:
>  variable 'device_id' set but not used [-Wunused-but-set-variable]
>
> drivers/scsi/pm8001/pm8001_sas.c: In function 'pm8001_abort_task':
> drivers/scsi/pm8001/pm8001_sas.c:1191:6: warning:
>  variable 'device_id' set but not used [-Wunused-but-set-variable]
>
> Signed-off-by: YueHaibing 
Acked-by: Jack Wang 
Thanks!
> ---
>  drivers/scsi/pm8001/pm8001_sas.c | 4 
>  1 file changed, 4 deletions(-)
>
> diff --git a/drivers/scsi/pm8001/pm8001_sas.c 
> b/drivers/scsi/pm8001/pm8001_sas.c
> index b1e7d26..84092e4 100644
> --- a/drivers/scsi/pm8001/pm8001_sas.c
> +++ b/drivers/scsi/pm8001/pm8001_sas.c
> @@ -1049,13 +1049,11 @@ int pm8001_I_T_nexus_event_handler(struct 
> domain_device *dev)
> struct pm8001_device *pm8001_dev;
> struct pm8001_hba_info *pm8001_ha;
> struct sas_phy *phy;
> -   u32 device_id = 0;
>
> if (!dev || !dev->lldd_dev)
> return -1;
>
> pm8001_dev = dev->lldd_dev;
> -   device_id = pm8001_dev->device_id;
> pm8001_ha = pm8001_find_ha_by_dev(dev);
>
> PM8001_EH_DBG(pm8001_ha,
> @@ -1188,7 +1186,6 @@ int pm8001_abort_task(struct sas_task *task)
>  {
> unsigned long flags;
> u32 tag;
> -   u32 device_id;
> struct domain_device *dev ;
> struct pm8001_hba_info *pm8001_ha;
> struct scsi_lun lun;
> @@ -1202,7 +1199,6 @@ int pm8001_abort_task(struct sas_task *task)
> dev = task->dev;
> pm8001_dev = dev->lldd_dev;
> pm8001_ha = pm8001_find_ha_by_dev(dev);
> -   device_id = pm8001_dev->device_id;
> phy_id = pm8001_dev->attached_phy;
> rc = pm8001_find_tag(task, );
> if (rc == 0) {
>


-- 
Jack Wang
Linux Kernel Developer

ProfitBricks GmbH
Greifswalder Str. 207
D - 10405 Berlin

Tel:   +49 30 577 008  042
Fax:  +49 30 577 008 299
Email:jinpu.w...@profitbricks.com
URL:  https://www.profitbricks.de

Sitz der Gesellschaft: Berlin
Registergericht: Amtsgericht Charlottenburg, HRB 125506 B
Geschäftsführer: Achim Weiss, Matthias Steinberg, Christoph Steffens

Re: [PATCH V2 1/4] pm80xx : Fix for phy enable/disable functionality.

[Jinpu Wang]

On Tue, Sep 11, 2018 at 10:48 AM Viswas G  wrote:
>
> From: Deepak Ukey 
>
> Added proper mask for phy id in mpi_phy_stop_resp().
>
> V2:
> -Initialized the PM8001F_RUN_TIME flag in
>  pm8001_pci_probe().
> -Differentiated PHY_LINK_UP state for SPC and SPCv
>  controller.
> -Used PHY_LINK_DOWN macro for phy state down.
>
> Signed-off-by: Deepak Ukey 
> Signed-off-by: Viswas G 
Acked-by: Jack Wang 
Thanks!
> ---
>  drivers/scsi/pm8001/pm8001_defs.h |  8 
>  drivers/scsi/pm8001/pm8001_hwi.c  |  3 ++-
>  drivers/scsi/pm8001/pm8001_hwi.h  |  4 
>  drivers/scsi/pm8001/pm8001_init.c |  3 ++-
>  drivers/scsi/pm8001/pm8001_sas.c  | 28 +---
>  drivers/scsi/pm8001/pm80xx_hwi.c  | 14 --
>  drivers/scsi/pm8001/pm80xx_hwi.h  |  6 --
>  7 files changed, 49 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/scsi/pm8001/pm8001_defs.h 
> b/drivers/scsi/pm8001/pm8001_defs.h
> index 199527dbaaa1..48e0624ecc68 100644
> --- a/drivers/scsi/pm8001/pm8001_defs.h
> +++ b/drivers/scsi/pm8001/pm8001_defs.h
> @@ -132,4 +132,12 @@ enum pm8001_hba_info_flags {
> PM8001F_RUN_TIME= (1U << 1),
>  };
>
> +/**
> + * Phy Status
> + */
> +#define PHY_LINK_DISABLE   0x00
> +#define PHY_LINK_DOWN  0x01
> +#define PHY_STATE_LINK_UP_SPCV 0x2
> +#define PHY_STATE_LINK_UP_SPC  0x1
> +
>  #endif
> diff --git a/drivers/scsi/pm8001/pm8001_hwi.c 
> b/drivers/scsi/pm8001/pm8001_hwi.c
> index 4dd6cad330e8..a14bf50a76d7 100644
> --- a/drivers/scsi/pm8001/pm8001_hwi.c
> +++ b/drivers/scsi/pm8001/pm8001_hwi.c
> @@ -3810,7 +3810,8 @@ static int mpi_hw_event(struct pm8001_hba_info 
> *pm8001_ha, void* piomb)
> " status = %x\n", status));
> if (status == 0) {
> phy->phy_state = 1;
> -   if (pm8001_ha->flags == PM8001F_RUN_TIME)
> +   if (pm8001_ha->flags == PM8001F_RUN_TIME &&
> +   phy->enable_completion != NULL)
> complete(phy->enable_completion);
> }
> break;
> diff --git a/drivers/scsi/pm8001/pm8001_hwi.h 
> b/drivers/scsi/pm8001/pm8001_hwi.h
> index e4867e690c84..6d91e2446542 100644
> --- a/drivers/scsi/pm8001/pm8001_hwi.h
> +++ b/drivers/scsi/pm8001/pm8001_hwi.h
> @@ -131,10 +131,6 @@
>  #define LINKRATE_30(0x02 << 8)
>  #define LINKRATE_60(0x04 << 8)
>
> -/* for phy state */
> -
> -#define PHY_STATE_LINK_UP_SPC  0x1
> -
>  /* for new SPC controllers MEMBASE III is shared between BIOS and DATA */
>  #define GSM_SM_BASE0x4F
>  struct mpi_msg_hdr{
> diff --git a/drivers/scsi/pm8001/pm8001_init.c 
> b/drivers/scsi/pm8001/pm8001_init.c
> index 7a697ca68501..501830caba21 100644
> --- a/drivers/scsi/pm8001/pm8001_init.c
> +++ b/drivers/scsi/pm8001/pm8001_init.c
> @@ -121,7 +121,7 @@ static void pm8001_phy_init(struct pm8001_hba_info 
> *pm8001_ha, int phy_id)
>  {
> struct pm8001_phy *phy = _ha->phy[phy_id];
> struct asd_sas_phy *sas_phy = >sas_phy;
> -   phy->phy_state = 0;
> +   phy->phy_state = PHY_LINK_DISABLE;
> phy->pm8001_ha = pm8001_ha;
> sas_phy->enabled = (phy_id < pm8001_ha->chip->n_phy) ? 1 : 0;
> sas_phy->class = SAS;
> @@ -1067,6 +1067,7 @@ static int pm8001_pci_probe(struct pci_dev *pdev,
> if (rc)
> goto err_out_shost;
> scsi_scan_host(pm8001_ha->shost);
> +   pm8001_ha->flags = PM8001F_RUN_TIME;
> return 0;
>
>  err_out_shost:
> diff --git a/drivers/scsi/pm8001/pm8001_sas.c 
> b/drivers/scsi/pm8001/pm8001_sas.c
> index 947d6017d004..d8249675371e 100644
> --- a/drivers/scsi/pm8001/pm8001_sas.c
> +++ b/drivers/scsi/pm8001/pm8001_sas.c
> @@ -157,9 +157,12 @@ int pm8001_phy_control(struct asd_sas_phy *sas_phy, enum 
> phy_func func,
> int rc = 0, phy_id = sas_phy->id;
> struct pm8001_hba_info *pm8001_ha = NULL;
> struct sas_phy_linkrates *rates;
> +   struct sas_ha_struct *sas_ha;
> +   struct pm8001_phy *phy;
> DECLARE_COMPLETION_ONSTACK(completion);
> unsigned long flags;
> pm8001_ha = sas_phy->ha->lldd_ha;
> +   phy = _ha->phy[phy_id];
> pm8001_ha->phy[phy_id].enable_completion = 
> switch (func) {
> case PHY_FUNC_SET_LINK_RATE:
> @@ -172,7 +175,7 @@ int pm8001_phy_control(struct asd_sas_phy *sas_phy, enum 
> phy_func func,
> pm8001_ha->phy[phy_id].maximum_linkrate =
> rates->maximum_linkrate;
> }
> -   if (pm8001_ha->phy[phy_id].phy_state == 0) {
> +   if (pm8001_ha->phy[phy_id].phy_state ==  PHY_LINK_DISABLE) {
> PM8001_CHIP_DISP->phy_start_req(pm8001_ha, phy_id);
> 

Re: [PATCH 4/4] pm80xx : Update driver version to 0.1.39

[Jinpu Wang]

On Wed, Sep 5, 2018 at 7:47 AM Viswas G  wrote:
>
> From: Deepak Ukey 
>
> Updated the driver version from 0.1.38 to 0.1.39.
>
> Signed-off-by: Deepak Ukey 
> Signed-off-by: Viswas G 
> ---
>  drivers/scsi/pm8001/pm8001_sas.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/pm8001/pm8001_sas.h 
> b/drivers/scsi/pm8001/pm8001_sas.h
> index 1816e35..f88b0d3 100644
> --- a/drivers/scsi/pm8001/pm8001_sas.h
> +++ b/drivers/scsi/pm8001/pm8001_sas.h
> @@ -58,7 +58,7 @@
>  #include "pm8001_defs.h"
>
>  #define DRV_NAME   "pm80xx"
> -#define DRV_VERSION"0.1.38"
> +#define DRV_VERSION"0.1.39"
>  #define PM8001_FAIL_LOGGING0x01 /* Error message logging */
>  #define PM8001_INIT_LOGGING0x02 /* driver init logging */
>  #define PM8001_DISC_LOGGING0x04 /* discovery layer logging */
> --
> 1.8.3.1
>

Acked-by: Jack Wang 
Thanks,
-- 
Jack Wang
Linux Kernel Developer

ProfitBricks GmbH
Greifswalder Str. 207
D - 10405 Berlin

Tel:   +49 30 577 008  042
Fax:  +49 30 577 008 299
Email:jinpu.w...@profitbricks.com
URL:  https://www.profitbricks.de

Sitz der Gesellschaft: Berlin
Registergericht: Amtsgericht Charlottenburg, HRB 125506 B
Geschäftsführer: Achim Weiss, Matthias Steinberg, Christoph Steffens

Re: [PATCH 3/4] pm80xx : Fixed system hang issue during kexec boot.

[Jinpu Wang]

On Wed, Sep 5, 2018 at 7:47 AM Viswas G  wrote:
>
> From: Deepak Ukey 
>
> When the firmware is not responding, execution of kexec boot
> causes a system hang. When firmware assertion happened, driver
> get notified with interrupt vector updated in MPI configuration
> table. Then, the driver will read scratchpad register and
> set controller_fatal_error flag to true.
>
> Signed-off-by: Deepak Ukey 
> Signed-off-by: Viswas G 
> ---
>  drivers/scsi/pm8001/pm8001_hwi.c |  6 +++
>  drivers/scsi/pm8001/pm8001_sas.c |  7 
>  drivers/scsi/pm8001/pm8001_sas.h |  1 +
>  drivers/scsi/pm8001/pm80xx_hwi.c | 80 
> +---
>  drivers/scsi/pm8001/pm80xx_hwi.h |  3 ++
>  5 files changed, 91 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/scsi/pm8001/pm8001_hwi.c 
> b/drivers/scsi/pm8001/pm8001_hwi.c
> index fcfb4f7..403ea8c 100644
> --- a/drivers/scsi/pm8001/pm8001_hwi.c
> +++ b/drivers/scsi/pm8001/pm8001_hwi.c
> @@ -1479,6 +1479,12 @@ u32 pm8001_mpi_msg_consume(struct pm8001_hba_info 
> *pm8001_ha,
> } else {
> u32 producer_index;
> void *pi_virt = circularQ->pi_virt;
> +   /* spurious interrupt during setup if
> +* kexec-ing and driver doing a doorbell access
> +* with the pre-kexec oq interrupt setup
> +*/
> +   if (!pi_virt)
> +   break;
> /* Update the producer index from SPC */
> producer_index = pm8001_read_32(pi_virt);
> circularQ->producer_index = 
> cpu_to_le32(producer_index);
> diff --git a/drivers/scsi/pm8001/pm8001_sas.c 
> b/drivers/scsi/pm8001/pm8001_sas.c
> index 719233c..f9c8f21 100644
> --- a/drivers/scsi/pm8001/pm8001_sas.c
> +++ b/drivers/scsi/pm8001/pm8001_sas.c
> @@ -384,6 +384,13 @@ static int pm8001_task_exec(struct sas_task *task,
> return 0;
> }
> pm8001_ha = pm8001_find_ha_by_dev(task->dev);
> +   if (pm8001_ha->controller_fatal_error) {
> +   struct task_status_struct *ts = >task_status;
> +
> +   ts->resp = SAS_TASK_UNDELIVERED;
> +   t->task_done(t);
> +   return 0;
> +   }
> PM8001_IO_DBG(pm8001_ha, pm8001_printk("pm8001_task_exec device \n 
> "));
> spin_lock_irqsave(_ha->lock, flags);
> do {
> diff --git a/drivers/scsi/pm8001/pm8001_sas.h 
> b/drivers/scsi/pm8001/pm8001_sas.h
> index 80b4dd6..1816e35 100644
> --- a/drivers/scsi/pm8001/pm8001_sas.h
> +++ b/drivers/scsi/pm8001/pm8001_sas.h
> @@ -538,6 +538,7 @@ struct pm8001_hba_info {
> u32 logging_level;
> u32 fw_status;
> u32 smp_exp_mode;
> +   boolcontroller_fatal_error;
> const struct firmware   *fw_image;
> struct isr_param irq_vector[PM8001_MAX_MSIX_VEC];
> u32 reset_in_progress;
> diff --git a/drivers/scsi/pm8001/pm80xx_hwi.c 
> b/drivers/scsi/pm8001/pm80xx_hwi.c
> index 17e74a3..6eec439 100644
> --- a/drivers/scsi/pm8001/pm80xx_hwi.c
> +++ b/drivers/scsi/pm8001/pm80xx_hwi.c
> @@ -577,6 +577,9 @@ static void update_main_config_table(struct 
> pm8001_hba_info *pm8001_ha)
> pm8001_ha->main_cfg_tbl.pm80xx_tbl.pcs_event_log_size);
> pm8001_mw32(address, MAIN_PCS_EVENT_LOG_OPTION,
> pm8001_ha->main_cfg_tbl.pm80xx_tbl.pcs_event_log_severity);
> +   /* Update Fatal error interrupt vector */
> +   pm8001_ha->main_cfg_tbl.pm80xx_tbl.fatal_err_interrupt |=
> +   ((pm8001_ha->number_of_intr - 1) << 
> 8);
> pm8001_mw32(address, MAIN_FATAL_ERROR_INTERRUPT,
> pm8001_ha->main_cfg_tbl.pm80xx_tbl.fatal_err_interrupt);
> pm8001_mw32(address, MAIN_EVENT_CRC_CHECK,
> @@ -1110,6 +1113,9 @@ static int pm80xx_chip_init(struct pm8001_hba_info 
> *pm8001_ha)
> return -EBUSY;
> }
>
> +   /* Initialize the controller fatal error flag */
> +   pm8001_ha->controller_fatal_error = false;
> +
> /* Initialize pci space address eg: mpi offset */
> init_pci_device_addresses(pm8001_ha);
> init_default_table_values(pm8001_ha);
> @@ -1218,13 +1224,17 @@ static int mpi_uninit_check(struct pm8001_hba_info 
> *pm8001_ha)
> u32 bootloader_state;
> u32 ibutton0, ibutton1;
>
> -   /* Check if MPI is in ready state to reset */
> -   if (mpi_uninit_check(pm8001_ha) != 0) {
> -   PM8001_FAIL_DBG(pm8001_ha,
> -   pm8001_printk("MPI state is not ready\n"));
> -   return -1;
> +   /* Process MPI table uninitialization only if FW is ready */
> +   if (!pm8001_ha->controller_fatal_error) {
> +   /* Check if MPI is in ready state to reset */
> + 

Re: [PATCH 2/4] pm80xx : Corrected dma_unmap_sg() parameter.

[Jinpu Wang]

On Wed, Sep 5, 2018 at 7:47 AM Viswas G  wrote:
>
> From: Deepak Ukey 
>
> For the function dma_unmap_sg(), the  parameter should be
> number of elements in the scatter list prior to the mapping, not
> after the mapping.
>
> Signed-off-by: Deepak Ukey 
> Signed-off-by: Viswas G 
> ---
>  drivers/scsi/pm8001/pm8001_sas.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/pm8001/pm8001_sas.c 
> b/drivers/scsi/pm8001/pm8001_sas.c
> index 96b173f..719233c 100644
> --- a/drivers/scsi/pm8001/pm8001_sas.c
> +++ b/drivers/scsi/pm8001/pm8001_sas.c
> @@ -476,7 +476,7 @@ static int pm8001_task_exec(struct sas_task *task,
> dev_printk(KERN_ERR, pm8001_ha->dev, "pm8001 exec failed[%d]!\n", rc);
> if (!sas_protocol_ata(t->task_proto))
> if (n_elem)
> -   dma_unmap_sg(pm8001_ha->dev, t->scatter, n_elem,
> +   dma_unmap_sg(pm8001_ha->dev, t->scatter, 
> t->num_scatter,
> t->data_dir);
>  out_done:
> spin_unlock_irqrestore(_ha->lock, flags);
> --
> 1.8.3.1
>
Acked-by: Jack Wang 
Thanks,
-- 
Jack Wang
Linux Kernel Developer

ProfitBricks GmbH
Greifswalder Str. 207
D - 10405 Berlin

Tel:   +49 30 577 008  042
Fax:  +49 30 577 008 299
Email:jinpu.w...@profitbricks.com
URL:  https://www.profitbricks.de

Sitz der Gesellschaft: Berlin
Registergericht: Amtsgericht Charlottenburg, HRB 125506 B
Geschäftsführer: Achim Weiss, Matthias Steinberg, Christoph Steffens

Re: [PATCH 1/4] pm80xx : Fix for phy enable/disable functionality.

[Jinpu Wang]

On Wed, Sep 5, 2018 at 7:47 AM Viswas G  wrote:
>
> From: Deepak Ukey 
>
> Added proper mask for phy id in mpi_phy_stop_resp().
>
> Signed-off-by: Deepak Ukey 
> Signed-off-by: Viswas G 
> ---
>  drivers/scsi/pm8001/pm8001_defs.h |  7 +++
>  drivers/scsi/pm8001/pm8001_hwi.c  |  4 ++--
>  drivers/scsi/pm8001/pm8001_init.c |  2 +-
>  drivers/scsi/pm8001/pm8001_sas.c  | 16 +---
>  drivers/scsi/pm8001/pm80xx_hwi.c  |  7 ---
>  drivers/scsi/pm8001/pm80xx_hwi.h  |  4 
>  6 files changed, 31 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/scsi/pm8001/pm8001_defs.h 
> b/drivers/scsi/pm8001/pm8001_defs.h
> index 199527d..6b8bc66 100644
> --- a/drivers/scsi/pm8001/pm8001_defs.h
> +++ b/drivers/scsi/pm8001/pm8001_defs.h
> @@ -132,4 +132,11 @@ enum pm8001_hba_info_flags {
> PM8001F_RUN_TIME= (1U << 1),
>  };
>
> +/**
> + * Phy Status
> + */
> +#define PHY_LINK_DISABLE   0x00
> +#define PHY_LINK_DOWN  0x01
PHY_LINK_DOWN is not used?
> +#define PHY_LINK_UP0x02
> +
>  #endif
> diff --git a/drivers/scsi/pm8001/pm8001_hwi.c 
> b/drivers/scsi/pm8001/pm8001_hwi.c
> index 4dd6cad..fcfb4f7 100644
> --- a/drivers/scsi/pm8001/pm8001_hwi.c
> +++ b/drivers/scsi/pm8001/pm8001_hwi.c
> @@ -3829,7 +3829,7 @@ static int mpi_hw_event(struct pm8001_hba_info 
> *pm8001_ha, void* piomb)
> pm8001_printk("HW_EVENT_PHY_STOP_STATUS "
> "status = %x\n", status));
> if (status == 0)
> -   phy->phy_state = 0;
> +   phy->phy_state = PHY_LINK_DISABLE;
> break;
> case HW_EVENT_SATA_SPINUP_HOLD:
> PM8001_MSG_DBG(pm8001_ha,
> @@ -3841,7 +3841,7 @@ static int mpi_hw_event(struct pm8001_hba_info 
> *pm8001_ha, void* piomb)
> pm8001_printk("HW_EVENT_PHY_DOWN\n"));
> sas_ha->notify_phy_event(>sas_phy, PHYE_LOSS_OF_SIGNAL);
> phy->phy_attached = 0;
> -   phy->phy_state = 0;
> +   phy->phy_state = PHY_LINK_DISABLE;
> hw_event_phy_down(pm8001_ha, piomb);
> break;
> case HW_EVENT_PORT_INVALID:
> diff --git a/drivers/scsi/pm8001/pm8001_init.c 
> b/drivers/scsi/pm8001/pm8001_init.c
> index 7a697ca..3dca1dc 100644
> --- a/drivers/scsi/pm8001/pm8001_init.c
> +++ b/drivers/scsi/pm8001/pm8001_init.c
> @@ -121,7 +121,7 @@ static void pm8001_phy_init(struct pm8001_hba_info 
> *pm8001_ha, int phy_id)
>  {
> struct pm8001_phy *phy = _ha->phy[phy_id];
> struct asd_sas_phy *sas_phy = >sas_phy;
> -   phy->phy_state = 0;
> +   phy->phy_state = PHY_LINK_DISABLE;
> phy->pm8001_ha = pm8001_ha;
> sas_phy->enabled = (phy_id < pm8001_ha->chip->n_phy) ? 1 : 0;
> sas_phy->class = SAS;
> diff --git a/drivers/scsi/pm8001/pm8001_sas.c 
> b/drivers/scsi/pm8001/pm8001_sas.c
> index 947d601..96b173f 100644
> --- a/drivers/scsi/pm8001/pm8001_sas.c
> +++ b/drivers/scsi/pm8001/pm8001_sas.c
> @@ -157,9 +157,12 @@ int pm8001_phy_control(struct asd_sas_phy *sas_phy, enum 
> phy_func func,
> int rc = 0, phy_id = sas_phy->id;
> struct pm8001_hba_info *pm8001_ha = NULL;
> struct sas_phy_linkrates *rates;
> +   struct sas_ha_struct *sas_ha;
> +   struct pm8001_phy *phy;
> DECLARE_COMPLETION_ONSTACK(completion);
> unsigned long flags;
> pm8001_ha = sas_phy->ha->lldd_ha;
> +   phy = _ha->phy[phy_id];
> pm8001_ha->phy[phy_id].enable_completion = 
> switch (func) {
> case PHY_FUNC_SET_LINK_RATE:
> @@ -172,7 +175,7 @@ int pm8001_phy_control(struct asd_sas_phy *sas_phy, enum 
> phy_func func,
> pm8001_ha->phy[phy_id].maximum_linkrate =
> rates->maximum_linkrate;
> }
> -   if (pm8001_ha->phy[phy_id].phy_state == 0) {
> +   if (pm8001_ha->phy[phy_id].phy_state ==  PHY_LINK_DISABLE) {
> PM8001_CHIP_DISP->phy_start_req(pm8001_ha, phy_id);
> wait_for_completion();
> }
> @@ -180,7 +183,7 @@ int pm8001_phy_control(struct asd_sas_phy *sas_phy, enum 
> phy_func func,
>   PHY_LINK_RESET);
> break;
> case PHY_FUNC_HARD_RESET:
> -   if (pm8001_ha->phy[phy_id].phy_state == 0) {
> +   if (pm8001_ha->phy[phy_id].phy_state == PHY_LINK_DISABLE) {
> PM8001_CHIP_DISP->phy_start_req(pm8001_ha, phy_id);
> wait_for_completion();
> }
> @@ -188,7 +191,7 @@ int pm8001_phy_control(struct asd_sas_phy *sas_phy, enum 
> phy_func func,
>   PHY_HARD_RESET);
> break;
> case PHY_FUNC_LINK_RESET:
> -   if (pm8001_ha->phy[phy_id].phy_state == 0) {
> +   if 

Re: [PATCH] scsi: pm80xx: fix spelling mistake "UNSORPORTED" -> "SUPPORTED"

[Jinpu Wang]

On Sat, May 26, 2018 at 4:42 PM, Colin King  wrote:
> From: Colin Ian King 
>
> Trivial fix to spelling mistake in pm8001_printk message text; also
> I believe NOT_UNSUPPORTED should probably be NOT_SUPPORTED. Also
> fix the indent of the pm8001_printk statement.
>
> Signed-off-by: Colin Ian King 
Thanks, Colin.

Acked-by: Jack Wang 
> ---
>  drivers/scsi/pm8001/pm8001_hwi.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/pm8001/pm8001_hwi.c 
> b/drivers/scsi/pm8001/pm8001_hwi.c
> index db88a8e7ee0e..4dd6cad330e8 100644
> --- a/drivers/scsi/pm8001/pm8001_hwi.c
> +++ b/drivers/scsi/pm8001/pm8001_hwi.c
> @@ -3607,7 +3607,7 @@ int pm8001_mpi_reg_resp(struct pm8001_hba_info 
> *pm8001_ha, void *piomb)
> break;
> default:
> PM8001_MSG_DBG(pm8001_ha,
> -
> pm8001_printk("DEVREG_FAILURE_DEVICE_TYPE_NOT_UNSORPORTED\n"));
> +   
> pm8001_printk("DEVREG_FAILURE_DEVICE_TYPE_NOT_SUPPORTED\n"));
> break;
> }
> complete(pm8001_dev->dcompletion);
> --
> 2.17.0
>



-- 
Jack Wang
Linux Kernel Developer

ProfitBricks GmbH
Greifswalder Str. 207
D - 10405 Berlin

Tel:   +49 30 577 008  042
Fax:  +49 30 577 008 299
Email:jinpu.w...@profitbricks.com
URL:  https://www.profitbricks.de

Sitz der Gesellschaft: Berlin
Registergericht: Amtsgericht Charlottenburg, HRB 125506 B
Geschäftsführer: Achim Weiss, Matthias Steinberg, Christoph Steffens

Re: [PATCH v2] block: ratelimite pr_err on IO path

[Jinpu Wang]

On Fri, Apr 13, 2018 at 6:59 PM, Martin K. Petersen
 wrote:
>
> Jinpu,
>
> [CC:ed the mpt3sas maintainers]
>
> The ratelimit patch is just an attempt to treat the symptom, not the
> cause.
Agree. If we can fix the root cause, it will be great.
>
>> Thanks for asking, we updated mpt3sas driver which enables DIX support
>> (prot_mask=0x7f), all disks are SATA SSDs, no DIF support.
>> After reboot, kernel reports the IO errors from all the drives behind
>> HBA, seems for almost every read IO, which turns the system unusable:
>> [   13.079375] sda: ref tag error at location 0 (rcvd 143196159)
>> [   13.079989] sda: ref tag error at location 937702912 (rcvd 143196159)
>> [   13.080233] sda: ref tag error at location 937703072 (rcvd 143196159)
>> [   13.080407] sda: ref tag error at location 0 (rcvd 143196159)
>> [   13.080594] sda: ref tag error at location 8 (rcvd 143196159)
>
> That sounds like a bug in the mpt3sas driver or firmware. I guess the
> HBA could conceivably be operating a SATA device as DIX Type 0 and strip
> the PI on the drive side. But that doesn't seem to be a particularly
> useful mode of operation.
>
> Jinpu: Which firmware are you running? Also, please send us the output
> of:
>
> sg_readcap -l /dev/sda
> sg_inq -x /dev/sda
> sg_vpd /dev/sda
>
Disks are INTEL SSDSC2BX48, directly attached to HBA.
LSISAS3008: FWVersion(13.00.00.00), ChipRevision(0x02), BiosVersion(08.11.00.00)
mpt3sas_cm2: Protocol=(Initiator,Target),
Capabilities=(TLR,EEDP,Snapshot Buffer,Diag Trace Buffer,Task Set
Full,NCQ)

jwang@x:~$ sudo sg_vpd /dev/sdz
Supported VPD pages VPD page:
  Supported VPD pages [sv]
  Unit serial number [sn]
  Device identification [di]
  Mode page policy [mpp]
  ATA information (SAT) [ai]
  Block limits (SBC) [bl]
  Block device characteristics (SBC) [bdc]
  Logical block provisioning (SBC) [lbpv]
jwang@x:~$ sudo sg_inq -x /dev/sdz
VPD INQUIRY: extended INQUIRY data page
inquiry: field in cdb illegal (page not supported)
jwang@x:~$ sudo sg_readcap -l /dev/sdz
Read Capacity results:
   Protection: prot_en=0, p_type=0, p_i_exponent=0
   Logical block provisioning: lbpme=1, lbprz=1
   Last logical block address=937703087 (0x37e436af), Number of
logical blocks=937703088
   Logical block length=512 bytes
   Logical blocks per physical block exponent=3 [so physical block
length=4096 bytes]
   Lowest aligned logical block address=0
Hence:
   Device size: 480103981056 bytes, 457862.8 MiB, 480.10 GB


> Broadcom: How is DIX supposed to work for SATA drives behind an mpt3sas
> controller?
>
> --
> Martin K. Petersen  Oracle Linux Engineering


Thanks!

-- 
Jack Wang
Linux Kernel Developer

ProfitBricks GmbH
Greifswalder Str. 207
D - 10405 Berlin

Re: [PATCH] sd: succeed check_event if device is not removable

[Jinpu Wang]

Hi James,

Thanks for prompt reply.

On Thu, Jan 18, 2018 at 5:32 PM, James Bottomley
 wrote:
> On Thu, 2018-01-18 at 17:22 +0100, Jack Wang wrote:
>> From: Jack Wang 
>>
>> The check_events interface was added for check if device changes,
>> mainly for device is removable eg. CDROM
>>
>> In sd_open, it checks if device is removable then check_disk_change.
>>
>> when the device is not removable, we can simple succeeds the call
>> without send TUR.
>>
>> Signed-off-by: Jack Wang 
>> ---
>>  drivers/scsi/sd.c | 4 
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
>> index ab75ebd..773ce81 100644
>> --- a/drivers/scsi/sd.c
>> +++ b/drivers/scsi/sd.c
>> @@ -1576,6 +1576,10 @@ static unsigned int sd_check_events(struct
>> gendisk *disk, unsigned int clearing)
>>   sdp = sdkp->device;
>>   SCSI_LOG_HLQUEUE(3, sd_printk(KERN_INFO, sdkp,
>> "sd_check_events\n"));
>>
>> + if (!sdp->removable) {
>> + retval = 0;
>> + goto out;
>> + }
>
> This looks like very much the wrong place to fix whatever problem
> you're seeing is.  We could simply avoid setting up the events work
> function in genhd.c:device_add_disk(), which would be way more
> efficient.  However, I think some devices do require being probed
> occasionally with a TUR because its the only way SCSI gets AENs (not
> that we make much use of them).
>
> So first of all, what's the actual problem?
>
> James
>

Eagle eyes! The real problem I want to workaround is deadlock between
check_event and device removal, reported here:
https://patchwork.kernel.org/patch/10057977/

We've see in our production during incident, storage failure lead scsi
error handle and
offlined both devices, so both multipaths down, raid1 failed one leg (the
dm device).

During incident recovery, when tried to delete the broken scsi device,
there are processes in D state.

I failed to reproduce on my test machines, this is why I want to
workaround the problem by avoiding TUR.

Thanks,
-- 
Jack Wang

Re: [PATCH] scsi: sas: Convert timers to use timer_setup()

[Jinpu Wang]

On Wed, Oct 25, 2017 at 12:08 PM, Kees Cook  wrote:
> In preparation for unconditionally passing the struct timer_list pointer to
> all timer callbacks, switch to using the new timer_setup() and from_timer()
> to pass the timer pointer explicitly. This requires adding a pointer to
> hold the timer's target task, as there isn't a link back from slow_task.
>
> Cc: John Garry 
> Cc: "James E.J. Bottomley" 
> Cc: "Martin K. Petersen" 
> Cc: Jack Wang 
> Cc: lindar_...@usish.com
> Cc: Jens Axboe 
> Cc: Hannes Reinecke 
> Cc: Johannes Thumshirn 
> Cc: Benjamin Block 
> Cc: Baoyou Xie 
> Cc: Wei Yongjun 
> Cc: linux-scsi@vger.kernel.org
> Signed-off-by: Kees Cook 
> ---
>  drivers/scsi/hisi_sas/hisi_sas.h   |  1 -
>  drivers/scsi/hisi_sas/hisi_sas_main.c  | 14 ++
>  drivers/scsi/hisi_sas/hisi_sas_v1_hw.c |  6 +++---
>  drivers/scsi/hisi_sas/hisi_sas_v2_hw.c | 24 +++-
>  drivers/scsi/hisi_sas/hisi_sas_v3_hw.c |  2 +-
>  drivers/scsi/libsas/sas_expander.c |  8 
>  drivers/scsi/libsas/sas_init.c |  3 ++-
>  drivers/scsi/libsas/sas_scsi_host.c|  2 +-
>  drivers/scsi/mvsas/mv_init.c   |  3 +--
>  drivers/scsi/mvsas/mv_sas.c| 15 +++
>  drivers/scsi/mvsas/mv_sas.h|  1 -
>  drivers/scsi/pm8001/pm8001_sas.c   | 11 +--
>  include/scsi/libsas.h  |  1 +
>  13 files changed, 42 insertions(+), 49 deletions(-)
>
> diff --git a/drivers/scsi/hisi_sas/hisi_sas.h 
> b/drivers/scsi/hisi_sas/hisi_sas.h
> index 07f4a4cfbec1..15692ea05ced 100644
> --- a/drivers/scsi/hisi_sas/hisi_sas.h
> +++ b/drivers/scsi/hisi_sas/hisi_sas.h
> @@ -103,7 +103,6 @@ struct hisi_sas_phy {
> struct hisi_sas_port*port;
> struct asd_sas_phy  sas_phy;
> struct sas_identify identify;
> -   struct timer_list   timer;
> struct work_struct  phyup_ws;
> u64 port_id; /* from hw */
> u64 dev_sas_addr;
> diff --git a/drivers/scsi/hisi_sas/hisi_sas_main.c 
> b/drivers/scsi/hisi_sas/hisi_sas_main.c
> index 9e2990268f00..0d772a83fbe4 100644
> --- a/drivers/scsi/hisi_sas/hisi_sas_main.c
> +++ b/drivers/scsi/hisi_sas/hisi_sas_main.c
> @@ -627,7 +627,6 @@ static void hisi_sas_phy_init(struct hisi_hba *hisi_hba, 
> int phy_no)
>
> phy->hisi_hba = hisi_hba;
> phy->port = NULL;
> -   init_timer(>timer);
> sas_phy->enabled = (phy_no < hisi_hba->n_phy) ? 1 : 0;
> sas_phy->class = SAS;
> sas_phy->iproto = SAS_PROTOCOL_ALL;
> @@ -792,9 +791,10 @@ static void hisi_sas_task_done(struct sas_task *task)
> complete(>slow_task->completion);
>  }
>
> -static void hisi_sas_tmf_timedout(unsigned long data)
> +static void hisi_sas_tmf_timedout(struct timer_list *t)
>  {
> -   struct sas_task *task = (struct sas_task *)data;
> +   struct sas_task_slow *slow = from_timer(slow, t, timer);
> +   struct sas_task *task = slow->task;
> unsigned long flags;
>
> spin_lock_irqsave(>task_state_lock, flags);
> @@ -833,8 +833,7 @@ static int hisi_sas_exec_internal_tmf_task(struct 
> domain_device *device,
> }
> task->task_done = hisi_sas_task_done;
>
> -   task->slow_task->timer.data = (unsigned long) task;
> -   task->slow_task->timer.function = hisi_sas_tmf_timedout;
> +   task->slow_task->timer.function = 
> (TIMER_FUNC_TYPE)hisi_sas_tmf_timedout;
> task->slow_task->timer.expires = jiffies + TASK_TIMEOUT*HZ;
> add_timer(>slow_task->timer);
>
> @@ -1445,8 +1444,7 @@ hisi_sas_internal_task_abort(struct hisi_hba *hisi_hba,
> task->dev = device;
> task->task_proto = device->tproto;
> task->task_done = hisi_sas_task_done;
> -   task->slow_task->timer.data = (unsigned long)task;
> -   task->slow_task->timer.function = hisi_sas_tmf_timedout;
> +   task->slow_task->timer.function = 
> (TIMER_FUNC_TYPE)hisi_sas_tmf_timedout;
> task->slow_task->timer.expires = jiffies + msecs_to_jiffies(110);
> add_timer(>slow_task->timer);
>
> @@ -1875,7 +1873,7 @@ static struct Scsi_Host *hisi_sas_shost_alloc(struct 
> platform_device *pdev,
> hisi_hba->shost = shost;
> SHOST_TO_SAS_HA(shost) = _hba->sha;
>
> -   init_timer(_hba->timer);
> +   timer_setup(_hba->timer, NULL, 0);
>
> if (hisi_sas_get_fw_info(hisi_hba) < 0)
> goto err_out;
> diff --git a/drivers/scsi/hisi_sas/hisi_sas_v1_hw.c 
> b/drivers/scsi/hisi_sas/hisi_sas_v1_hw.c
> index 08eca20b0b81..9385554e43a6 100644
> --- a/drivers/scsi/hisi_sas/hisi_sas_v1_hw.c
> +++ 

Re: [PATCH V4 0/9] pm80xx updates

[Jinpu Wang]

On Wed, Oct 18, 2017 at 8:09 AM, Viswas G  wrote:
>
> This patch set include some bug fixes and enhancement for pm80xx driver.
>
> Changes from V3:
> - Fixed kbuild warnings for patch 4 and 5.
>
> Changes from V2:
> - Corrected date.
>
> Changes from V1:
> - sas_identify_frame_local structure moved to pm80xx_hwi.h
> - sata abort handling patch split to four patches.
> - tag allocation for phy control request.
> - cleanup in pm8001_abort_task function.
> - modified port reset timer value for PM8006 card
> - corrected SATA abort handling sequence.
>
>
> Viswas G (9):
>   pm80xx : redefine sas_identify_frame structure
>   pm80xx : ILA and inactive firmware version through sysfs
>   pm80xx : Different SAS addresses for phys.
>   pm80xx : tag allocation for phy control request.
>   pm80xx : cleanup in pm8001_abort_task function.
>   pm80xx : modified port reset timer value for PM8006  card
>   pm80xx : corrected SATA abort handling sequence.
>   pm80xx : panic on ncq error cleaning up the read log.
>   pm80xx : corrected linkrate value.
>
>  drivers/scsi/pm8001/pm8001_ctl.c  |  54 +
>  drivers/scsi/pm8001/pm8001_hwi.c  |  11 +++-
>  drivers/scsi/pm8001/pm8001_init.c |  13 +++--
>  drivers/scsi/pm8001/pm8001_sas.c  | 119 
> +-
>  drivers/scsi/pm8001/pm8001_sas.h  |  10 
>  drivers/scsi/pm8001/pm80xx_hwi.c  |  62 
>  drivers/scsi/pm8001/pm80xx_hwi.h  | 102 +++-
>  7 files changed, 313 insertions(+), 58 deletions(-)
>
> --
> 2.12.3
>
Thanks Viswas for the effort. I think it's good to go.

Jack Wang

Re: [PATCH V3 4/9] pm80xx : tag allocation for phy control request.

[Jinpu Wang]

On Wed, Sep 20, 2017 at 5:18 PM, kbuild test robot  wrote:
> Hi Viswas,
>
> [auto build test WARNING on mkp-scsi/for-next]
> [also build test WARNING on v4.14-rc1 next-20170920]
> [if your patch is applied to the wrong git tree, please drop us a note to 
> help improve the system]
>
> url:
> https://github.com/0day-ci/linux/commits/Viswas-G/pm80xx-updates/20170920-202506
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git for-next
> config: x86_64-randconfig-a0-09202136 (attached as .config)
> compiler: gcc-4.4 (Debian 4.4.7-8) 4.4.7
> reproduce:
> # save the attached .config to linux build tree
> make ARCH=x86_64
>
> All warnings (new ones prefixed by >>):
>
>drivers/scsi/pm8001/pm80xx_hwi.c: In function 'pm80xx_chip_phy_ctl_req':
>>> drivers/scsi/pm8001/pm80xx_hwi.c:4507: warning: unused variable 'ret'
>
> vim +/ret +4507 drivers/scsi/pm8001/pm80xx_hwi.c

Indeed, ret is unused, Viswas, could you send a update patch to fix
the warning in this patch and patch5.

Thanks
>
> f5860992d Sakthivel K 2013-04-17  4492
> f5860992d Sakthivel K 2013-04-17  4493  /**
> f5860992d Sakthivel K 2013-04-17  4494   * pm80xx_chip_phy_ctl_req - support 
> the local phy operation
> f5860992d Sakthivel K 2013-04-17  4495   * @pm8001_ha: our hba card 
> information.
> f5860992d Sakthivel K 2013-04-17  4496   * @num: the inbound queue number
> f5860992d Sakthivel K 2013-04-17  4497   * @phy_id: the phy id which we 
> wanted to operate
> f5860992d Sakthivel K 2013-04-17  4498   * @phy_op:
> f5860992d Sakthivel K 2013-04-17  4499   */
> f5860992d Sakthivel K 2013-04-17  4500  static int 
> pm80xx_chip_phy_ctl_req(struct pm8001_hba_info *pm8001_ha,
> f5860992d Sakthivel K 2013-04-17  4501  u32 phyId, u32 phy_op)
> f5860992d Sakthivel K 2013-04-17  4502  {
> 18e9bdf60 Viswas G2017-09-19  4503  u32 tag;
> 18e9bdf60 Viswas G2017-09-19  4504  int rc;
> f5860992d Sakthivel K 2013-04-17  4505  struct local_phy_ctl_req 
> payload;
> f5860992d Sakthivel K 2013-04-17  4506  struct inbound_queue_table 
> *circularQ;
> f5860992d Sakthivel K 2013-04-17 @4507  int ret;
> f5860992d Sakthivel K 2013-04-17  4508  u32 opc = 
> OPC_INB_LOCAL_PHY_CONTROL;
> f5860992d Sakthivel K 2013-04-17  4509  memset(, 0, 
> sizeof(payload));
> 18e9bdf60 Viswas G2017-09-19  4510  rc = 
> pm8001_tag_alloc(pm8001_ha, );
> 18e9bdf60 Viswas G2017-09-19  4511  if (rc)
> 18e9bdf60 Viswas G2017-09-19  4512  return rc;
> f5860992d Sakthivel K 2013-04-17  4513  circularQ = 
> _ha->inbnd_q_tbl[0];
> 18e9bdf60 Viswas G2017-09-19  4514  payload.tag = 
> cpu_to_le32(tag);
> f5860992d Sakthivel K 2013-04-17  4515  payload.phyop_phyid =
> f5860992d Sakthivel K 2013-04-17  4516  cpu_to_le32(((phy_op 
> & 0xFF) << 8) | (phyId & 0xFF));
> 18e9bdf60 Viswas G2017-09-19  4517  return 
> pm8001_mpi_build_cmd(pm8001_ha, circularQ, opc, , 0);
> f5860992d Sakthivel K 2013-04-17  4518  }
> f5860992d Sakthivel K 2013-04-17  4519
>
> :: The code at line 4507 was first introduced by commit
> :: f5860992db55c9e36b0f120dff73f0c34abe510d [SCSI] pm80xx: Added SPCv/ve 
> specific hardware functionalities and relevant changes in common files
>
> :: TO: Sakthivel K 
> :: CC: James Bottomley 
>
> ---
> 0-DAY kernel test infrastructureOpen Source Technology Center
> https://lists.01.org/pipermail/kbuild-all   Intel Corporation



-- 
Jack Wang
Linux Kernel Developer

ProfitBricks GmbH
Greifswalder Str. 207
D - 10405 Berlin

Tel:   +49 30 577 008  042
Fax:  +49 30 577 008 299
Email:jinpu.w...@profitbricks.com
URL:  https://www.profitbricks.de

Sitz der Gesellschaft: Berlin
Registergericht: Amtsgericht Charlottenburg, HRB 125506 B
Geschäftsführer: Achim Weiss

Re: [PATCH V2 5/9] pm80xx : cleanup in pm8001_abort_task function.

[Jinpu Wang]

On Wed, Feb 18, 2015 at 12:36 AM, Viswas G  wrote:
> Signed-off-by: Deepak Ukey 
> Signed-off-by: Viswas G 
> ---
>  drivers/scsi/pm8001/pm8001_sas.c | 49 
> +++-
>  1 file changed, 13 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/scsi/pm8001/pm8001_sas.c 
> b/drivers/scsi/pm8001/pm8001_sas.c
> index ce584c31d36e..e80b0542a67f 100644
> --- a/drivers/scsi/pm8001/pm8001_sas.c
> +++ b/drivers/scsi/pm8001/pm8001_sas.c
> @@ -1159,40 +1159,35 @@ int pm8001_query_task(struct sas_task *task)
>  int pm8001_abort_task(struct sas_task *task)
>  {
> unsigned long flags;
> -   u32 tag = 0xdeadbeef;
> +   u32 tag;
> u32 device_id;
> struct domain_device *dev ;
> -   struct pm8001_hba_info *pm8001_ha = NULL;
> +   struct pm8001_hba_info *pm8001_ha;
> struct pm8001_ccb_info *ccb;
> struct scsi_lun lun;
> struct pm8001_device *pm8001_dev;
> struct pm8001_tmf_task tmf_task;
> int rc = TMF_RESP_FUNC_FAILED;
> if (unlikely(!task || !task->lldd_task || !task->dev))
> -   return rc;
> +   return TMF_RESP_FUNC_FAILED;
> +   dev = task->dev;
> +   pm8001_dev = dev->lldd_dev;
> +   pm8001_ha = pm8001_find_ha_by_dev(dev);
> +   device_id = pm8001_dev->device_id;
> +   rc = pm8001_find_tag(task, );
> +   if (rc == 0) {
> +   pm8001_printk("no tag for task:%p\n", task);
> +   return TMF_RESP_FUNC_FAILED;
> +   }
> spin_lock_irqsave(>task_state_lock, flags);
> if (task->task_state_flags & SAS_TASK_STATE_DONE) {
> spin_unlock_irqrestore(>task_state_lock, flags);
> -   rc = TMF_RESP_FUNC_COMPLETE;
> -   goto out;
> +   return TMF_RESP_FUNC_COMPLETE;
> }
> spin_unlock_irqrestore(>task_state_lock, flags);
> if (task->task_proto & SAS_PROTOCOL_SSP) {
> struct scsi_cmnd *cmnd = task->uldd_task;
> -   dev = task->dev;
> -   ccb = task->lldd_task;
> -   pm8001_dev = dev->lldd_dev;
> -   pm8001_ha = pm8001_find_ha_by_dev(dev);
> int_to_scsilun(cmnd->device->lun, );
> -   rc = pm8001_find_tag(task, );
> -   if (rc == 0) {
> -   printk(KERN_INFO "No such tag in %s\n", __func__);
> -   rc = TMF_RESP_FUNC_FAILED;
> -   return rc;
> -   }
> -   device_id = pm8001_dev->device_id;
> -   PM8001_EH_DBG(pm8001_ha,
> -   pm8001_printk("abort io to deviceid= %d\n", 
> device_id));
> tmf_task.tmf = TMF_ABORT_TASK;
> tmf_task.tag_of_task_to_be_managed = tag;
> rc = pm8001_issue_ssp_tmf(dev, lun.scsi_lun, _task);
> @@ -1200,28 +1195,10 @@ int pm8001_abort_task(struct sas_task *task)
> pm8001_dev->sas_device, 0, tag);
> } else if (task->task_proto & SAS_PROTOCOL_SATA ||
> task->task_proto & SAS_PROTOCOL_STP) {
> -   dev = task->dev;
> -   pm8001_dev = dev->lldd_dev;
> -   pm8001_ha = pm8001_find_ha_by_dev(dev);
> -   rc = pm8001_find_tag(task, );
> -   if (rc == 0) {
> -   printk(KERN_INFO "No such tag in %s\n", __func__);
> -   rc = TMF_RESP_FUNC_FAILED;
> -   return rc;
> -   }
> rc = pm8001_exec_internal_task_abort(pm8001_ha, pm8001_dev,
> pm8001_dev->sas_device, 0, tag);
> } else if (task->task_proto & SAS_PROTOCOL_SMP) {
> /* SMP */
> -   dev = task->dev;
> -   pm8001_dev = dev->lldd_dev;
> -   pm8001_ha = pm8001_find_ha_by_dev(dev);
> -   rc = pm8001_find_tag(task, );
> -   if (rc == 0) {
> -   printk(KERN_INFO "No such tag in %s\n", __func__);
> -   rc = TMF_RESP_FUNC_FAILED;
> -   return rc;
> -   }
> rc = pm8001_exec_internal_task_abort(pm8001_ha, pm8001_dev,
> pm8001_dev->sas_device, 0, tag);
>
> --
> 2.12.3
>

Thanks,
Acked-by: Jack Wang 

-- 
Jack Wang
Linux Kernel Developer

ProfitBricks GmbH
Greifswalder Str. 207
D - 10405 Berlin

Tel:   +49 30 577 008  042
Fax:  +49 30 577 008 299
Email:jinpu.w...@profitbricks.com
URL:  https://www.profitbricks.de

Sitz der Gesellschaft: Berlin
Registergericht: Amtsgericht Charlottenburg, HRB 125506 B
Geschäftsführer: Achim Weiss

Re: [PATCH V2 8/9] pm80xx : panic on ncq error cleaning up the read log.

[Jinpu Wang]

On Wed, Feb 18, 2015 at 12:36 AM, Viswas G  wrote:
> when there's an error in 'ncq mode' the host has to read the ncq
> error log (10h) to clear the error state. however, the ccb that
> is setup for doing this doesn't setup the ccb so that the
> previous state is cleared. if the ccb was previously used for an IO
> n_elems is set and pm8001_ccb_task_free() treats this as the signal
> to go free a scatter-gather list (that's already been free-ed).
>
> Signed-off-by: Deepak Ukey 
> Signed-off-by: Viswas G 
> ---
>  drivers/scsi/pm8001/pm80xx_hwi.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/scsi/pm8001/pm80xx_hwi.c 
> b/drivers/scsi/pm8001/pm80xx_hwi.c
> index 92d2045dea68..f2c0839afbe3 100644
> --- a/drivers/scsi/pm8001/pm80xx_hwi.c
> +++ b/drivers/scsi/pm8001/pm80xx_hwi.c
> @@ -1489,6 +1489,7 @@ static void pm80xx_send_read_log(struct pm8001_hba_info 
> *pm8001_ha,
> ccb->device = pm8001_ha_dev;
> ccb->ccb_tag = ccb_tag;
> ccb->task = task;
> +   ccb->n_elem = 0;
> pm8001_ha_dev->id |= NCQ_READ_LOG_FLAG;
> pm8001_ha_dev->id |= NCQ_2ND_RLE_FLAG;
>
> --
> 2.12.3
>

Thanks,
Acked-by: Jack Wang 

-- 
Jack Wang
Linux Kernel Developer

ProfitBricks GmbH
Greifswalder Str. 207
D - 10405 Berlin

Tel:   +49 30 577 008  042
Fax:  +49 30 577 008 299
Email:jinpu.w...@profitbricks.com
URL:  https://www.profitbricks.de

Sitz der Gesellschaft: Berlin
Registergericht: Amtsgericht Charlottenburg, HRB 125506 B
Geschäftsführer: Achim Weiss

Re: [PATCH V2 7/9] pm80xx : corrected SATA abort handling sequence.

[Jinpu Wang]

On Wed, Feb 18, 2015 at 12:36 AM, Viswas G  wrote:
> Modified SATA abort handling with following steps:
> 1) Set device state as recovery.
> 2) Send phy reset.
> 3) Wait for reset completion.
> 4) After successful reset, abort all IO's to the device.
> 5) After aborting all IO's to device, set device state as operational.
>
> Signed-off-by: Deepak Ukey 
> Signed-off-by: Viswas G 
> ---
>  drivers/scsi/pm8001/pm8001_hwi.c |  8 -
>  drivers/scsi/pm8001/pm8001_sas.c | 71 
> ++--
>  drivers/scsi/pm8001/pm8001_sas.h |  8 +
>  drivers/scsi/pm8001/pm80xx_hwi.c | 36 
>  4 files changed, 113 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/scsi/pm8001/pm8001_hwi.c 
> b/drivers/scsi/pm8001/pm8001_hwi.c
> index bc4a6f649ec9..db88a8e7ee0e 100644
> --- a/drivers/scsi/pm8001/pm8001_hwi.c
> +++ b/drivers/scsi/pm8001/pm8001_hwi.c
> @@ -3209,10 +3209,16 @@ int pm8001_mpi_local_phy_ctl(struct pm8001_hba_info 
> *pm8001_ha, void *piomb)
> PM8001_MSG_DBG(pm8001_ha,
> pm8001_printk("%x phy execute %x phy op failed!\n",
> phy_id, phy_op));
> -   } else
> +   } else {
> PM8001_MSG_DBG(pm8001_ha,
> pm8001_printk("%x phy execute %x phy op success!\n",
> phy_id, phy_op));
> +   pm8001_ha->phy[phy_id].reset_success = true;
> +   }
> +   if (pm8001_ha->phy[phy_id].enable_completion) {
> +   complete(pm8001_ha->phy[phy_id].enable_completion);
> +   pm8001_ha->phy[phy_id].enable_completion = NULL;
> +   }
> pm8001_tag_free(pm8001_ha, tag);
> return 0;
>  }
> diff --git a/drivers/scsi/pm8001/pm8001_sas.c 
> b/drivers/scsi/pm8001/pm8001_sas.c
> index e80b0542a67f..60d5bec5c45e 100644
> --- a/drivers/scsi/pm8001/pm8001_sas.c
> +++ b/drivers/scsi/pm8001/pm8001_sas.c
> @@ -1167,13 +1167,16 @@ int pm8001_abort_task(struct sas_task *task)
> struct scsi_lun lun;
> struct pm8001_device *pm8001_dev;
> struct pm8001_tmf_task tmf_task;
> -   int rc = TMF_RESP_FUNC_FAILED;
> +   int rc = TMF_RESP_FUNC_FAILED, ret;
> +   u32 phy_id;
> +   struct sas_task_slow slow_task;
> if (unlikely(!task || !task->lldd_task || !task->dev))
> return TMF_RESP_FUNC_FAILED;
> dev = task->dev;
> pm8001_dev = dev->lldd_dev;
> pm8001_ha = pm8001_find_ha_by_dev(dev);
> device_id = pm8001_dev->device_id;
> +   phy_id = pm8001_dev->attached_phy;
> rc = pm8001_find_tag(task, );
> if (rc == 0) {
> pm8001_printk("no tag for task:%p\n", task);
> @@ -1184,6 +1187,11 @@ int pm8001_abort_task(struct sas_task *task)
> spin_unlock_irqrestore(>task_state_lock, flags);
> return TMF_RESP_FUNC_COMPLETE;
> }
> +   task->task_state_flags |= SAS_TASK_STATE_ABORTED;
> +   if (task->slow_task == NULL) {
> +   init_completion(_task.completion);
> +   task->slow_task = _task;
> +   }
> spin_unlock_irqrestore(>task_state_lock, flags);
> if (task->task_proto & SAS_PROTOCOL_SSP) {
> struct scsi_cmnd *cmnd = task->uldd_task;
> @@ -1195,8 +1203,61 @@ int pm8001_abort_task(struct sas_task *task)
> pm8001_dev->sas_device, 0, tag);
> } else if (task->task_proto & SAS_PROTOCOL_SATA ||
> task->task_proto & SAS_PROTOCOL_STP) {
> -   rc = pm8001_exec_internal_task_abort(pm8001_ha, pm8001_dev,
> -   pm8001_dev->sas_device, 0, tag);
> +   if (pm8001_ha->chip_id == chip_8006) {
> +   DECLARE_COMPLETION_ONSTACK(completion_reset);
> +   DECLARE_COMPLETION_ONSTACK(completion);
> +   struct pm8001_phy *phy = pm8001_ha->phy + phy_id;
> +   /* 1. Set Device state as Recovery*/
> +   pm8001_dev->setds_completion = 
> +   PM8001_CHIP_DISP->set_dev_state_req(pm8001_ha,
> +   pm8001_dev, 0x03);
> +   wait_for_completion();
> +   /* 2. Send Phy Control Hard Reset */
> +   reinit_completion();
> +   phy->reset_success = false;
> +   phy->enable_completion = 
> +   phy->reset_completion = _reset;
> +   ret = PM8001_CHIP_DISP->phy_ctl_req(pm8001_ha, phy_id,
> +   PHY_HARD_RESET);
> +   if (ret)
> +   goto out;
> +   PM8001_MSG_DBG(pm8001_ha,
> +   pm8001_printk("Waiting for local phy ctl\n"));
> +   wait_for_completion();
> +   

Re: [PATCH V2 6/9] pm80xx : modified port reset timer value for PM8006 card

[Jinpu Wang]

On Wed, Feb 18, 2015 at 12:36 AM, Viswas G  wrote:
> Added port reset timer value as 2000ms for PM8006 sata controller.
>
> Signed-off-by: Deepak Ukey 
> Signed-off-by: Viswas G 
> ---
>  drivers/scsi/pm8001/pm80xx_hwi.c | 6 ++
>  1 file changed, 6 insertions(+)
>
> diff --git a/drivers/scsi/pm8001/pm80xx_hwi.c 
> b/drivers/scsi/pm8001/pm80xx_hwi.c
> index baab8a19c78e..8f1f5dc77d71 100644
> --- a/drivers/scsi/pm8001/pm80xx_hwi.c
> +++ b/drivers/scsi/pm8001/pm80xx_hwi.c
> @@ -597,6 +597,12 @@ static void update_main_config_table(struct 
> pm8001_hba_info *pm8001_ha)
> pm8001_ha->main_cfg_tbl.pm80xx_tbl.port_recovery_timer &= 0x;
> pm8001_ha->main_cfg_tbl.pm80xx_tbl.port_recovery_timer |=
> PORT_RECOVERY_TIMEOUT;
> +   if (pm8001_ha->chip_id == chip_8006) {
> +   pm8001_ha->main_cfg_tbl.pm80xx_tbl.port_recovery_timer &=
> +   0x;
> +   pm8001_ha->main_cfg_tbl.pm80xx_tbl.port_recovery_timer |=
> +   0x14;
> +   }
> pm8001_mw32(address, MAIN_PORT_RECOVERY_TIMER,
> 
> pm8001_ha->main_cfg_tbl.pm80xx_tbl.port_recovery_timer);
>  }
> --
> 2.12.3
>

Thanks,
Acked-by: Jack Wang 

-- 
Jack Wang
Linux Kernel Developer

ProfitBricks GmbH
Greifswalder Str. 207
D - 10405 Berlin

Tel:   +49 30 577 008  042
Fax:  +49 30 577 008 299
Email:jinpu.w...@profitbricks.com
URL:  https://www.profitbricks.de

Sitz der Gesellschaft: Berlin
Registergericht: Amtsgericht Charlottenburg, HRB 125506 B
Geschäftsführer: Achim Weiss

Re: [PATCH V2 4/9] pm80xx : tag allocation for phy control request.

[Jinpu Wang]

On Wed, Feb 18, 2015 at 12:36 AM, Viswas G  wrote:
> tag is taken from the tag pool instead of using the hardcoded
> tag value(1).
>
> Signed-off-by: Deepak Ukey 
> Signed-off-by: Viswas G 
> ---
>  drivers/scsi/pm8001/pm8001_hwi.c |  3 +++
>  drivers/scsi/pm8001/pm80xx_hwi.c | 10 +++---
>  2 files changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/scsi/pm8001/pm8001_hwi.c 
> b/drivers/scsi/pm8001/pm8001_hwi.c
> index 10546faac58c..bc4a6f649ec9 100644
> --- a/drivers/scsi/pm8001/pm8001_hwi.c
> +++ b/drivers/scsi/pm8001/pm8001_hwi.c
> @@ -3198,11 +3198,13 @@ pm8001_mpi_get_nvmd_resp(struct pm8001_hba_info 
> *pm8001_ha, void *piomb)
>
>  int pm8001_mpi_local_phy_ctl(struct pm8001_hba_info *pm8001_ha, void *piomb)
>  {
> +   u32 tag;
> struct local_phy_ctl_resp *pPayload =
> (struct local_phy_ctl_resp *)(piomb + 4);
> u32 status = le32_to_cpu(pPayload->status);
> u32 phy_id = le32_to_cpu(pPayload->phyop_phyid) & ID_BITS;
> u32 phy_op = le32_to_cpu(pPayload->phyop_phyid) & OP_BITS;
> +   tag = le32_to_cpu(pPayload->tag);
> if (status != 0) {
> PM8001_MSG_DBG(pm8001_ha,
> pm8001_printk("%x phy execute %x phy op failed!\n",
> @@ -3211,6 +3213,7 @@ int pm8001_mpi_local_phy_ctl(struct pm8001_hba_info 
> *pm8001_ha, void *piomb)
> PM8001_MSG_DBG(pm8001_ha,
> pm8001_printk("%x phy execute %x phy op success!\n",
> phy_id, phy_op));
> +   pm8001_tag_free(pm8001_ha, tag);
> return 0;
>  }
>
> diff --git a/drivers/scsi/pm8001/pm80xx_hwi.c 
> b/drivers/scsi/pm8001/pm80xx_hwi.c
> index 2b26445d1b97..baab8a19c78e 100644
> --- a/drivers/scsi/pm8001/pm80xx_hwi.c
> +++ b/drivers/scsi/pm8001/pm80xx_hwi.c
> @@ -4500,17 +4500,21 @@ static int pm80xx_chip_reg_dev_req(struct 
> pm8001_hba_info *pm8001_ha,
>  static int pm80xx_chip_phy_ctl_req(struct pm8001_hba_info *pm8001_ha,
> u32 phyId, u32 phy_op)
>  {
> +   u32 tag;
> +   int rc;
> struct local_phy_ctl_req payload;
> struct inbound_queue_table *circularQ;
> int ret;
> u32 opc = OPC_INB_LOCAL_PHY_CONTROL;
> memset(, 0, sizeof(payload));
> +   rc = pm8001_tag_alloc(pm8001_ha, );
> +   if (rc)
> +   return rc;
> circularQ = _ha->inbnd_q_tbl[0];
> -   payload.tag = cpu_to_le32(1);
> +   payload.tag = cpu_to_le32(tag);
> payload.phyop_phyid =
> cpu_to_le32(((phy_op & 0xFF) << 8) | (phyId & 0xFF));
> -   ret = pm8001_mpi_build_cmd(pm8001_ha, circularQ, opc, , 0);
> -   return ret;
> +   return pm8001_mpi_build_cmd(pm8001_ha, circularQ, opc, , 0);
>  }
>
>  static u32 pm80xx_chip_is_our_interupt(struct pm8001_hba_info *pm8001_ha)
> --
> 2.12.3
>

Thanks,
Acked-by: Jack Wang 

-- 
Jack Wang
Linux Kernel Developer

ProfitBricks GmbH
Greifswalder Str. 207
D - 10405 Berlin

Tel:   +49 30 577 008  042
Fax:  +49 30 577 008 299
Email:jinpu.w...@profitbricks.com
URL:  https://www.profitbricks.de

Sitz der Gesellschaft: Berlin
Registergericht: Amtsgericht Charlottenburg, HRB 125506 B
Geschäftsführer: Achim Weiss

Re: [PATCH V2 1/9] pm80xx : redefine sas_identify_frame structure

[Jinpu Wang]

On Wed, Feb 18, 2015 at 12:36 AM, Viswas G  wrote:
> sas_identify structure defined by pm80xx doesn't have CRC field.
> So added a new sas_identify structure without CRC.
>
> v2:
> - Since the structure changes is applicable for only pm80xx,
>   sas_identify_frame_local structure moved to pm80xx_hwi.h.
>
> Signed-off-by: Raj Dinesh 
> Signed-off-by: Viswas G 
> ---
>  drivers/scsi/pm8001/pm80xx_hwi.h | 98 
> +++-
>  1 file changed, 97 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/pm8001/pm80xx_hwi.h 
> b/drivers/scsi/pm8001/pm80xx_hwi.h
> index 7a443bad6163..82b8cf581da9 100644
> --- a/drivers/scsi/pm8001/pm80xx_hwi.h
> +++ b/drivers/scsi/pm8001/pm80xx_hwi.h
> @@ -229,6 +229,102 @@
>  #define IT_NEXUS_TIMEOUT   0x7D0
>  #define PORT_RECOVERY_TIMEOUT  ((IT_NEXUS_TIMEOUT/100) + 30)
>
> +#ifdef __LITTLE_ENDIAN_BITFIELD
> +struct sas_identify_frame_local {
> +   /* Byte 0 */
> +   u8  frame_type:4;
> +   u8  dev_type:3;
> +   u8  _un0:1;
> +
> +   /* Byte 1 */
> +   u8  _un1;
> +
> +   /* Byte 2 */
> +   union {
> +   struct {
> +   u8  _un20:1;
> +   u8  smp_iport:1;
> +   u8  stp_iport:1;
> +   u8  ssp_iport:1;
> +   u8  _un247:4;
> +   };
> +   u8 initiator_bits;
> +   };
> +
> +   /* Byte 3 */
> +   union {
> +   struct {
> +   u8  _un30:1;
> +   u8 smp_tport:1;
> +   u8 stp_tport:1;
> +   u8 ssp_tport:1;
> +   u8 _un347:4;
> +   };
> +   u8 target_bits;
> +   };
> +
> +   /* Byte 4 - 11 */
> +   u8 _un4_11[8];
> +
> +   /* Byte 12 - 19 */
> +   u8 sas_addr[SAS_ADDR_SIZE];
> +
> +   /* Byte 20 */
> +   u8 phy_id;
> +
> +   u8 _un21_27[7];
> +
> +} __packed;
> +
> +#elif defined(__BIG_ENDIAN_BITFIELD)
> +struct sas_identify_frame_local {
> +   /* Byte 0 */
> +   u8  _un0:1;
> +   u8  dev_type:3;
> +   u8  frame_type:4;
> +
> +   /* Byte 1 */
> +   u8  _un1;
> +
> +   /* Byte 2 */
> +   union {
> +   struct {
> +   u8  _un247:4;
> +   u8  ssp_iport:1;
> +   u8  stp_iport:1;
> +   u8  smp_iport:1;
> +   u8  _un20:1;
> +   };
> +   u8 initiator_bits;
> +   };
> +
> +   /* Byte 3 */
> +   union {
> +   struct {
> +   u8 _un347:4;
> +   u8 ssp_tport:1;
> +   u8 stp_tport:1;
> +   u8 smp_tport:1;
> +   u8 _un30:1;
> +   };
> +   u8 target_bits;
> +   };
> +
> +   /* Byte 4 - 11 */
> +   u8 _un4_11[8];
> +
> +   /* Byte 12 - 19 */
> +   u8 sas_addr[SAS_ADDR_SIZE];
> +
> +   /* Byte 20 */
> +   u8 phy_id;
> +
> +   u8 _un21_27[7];
> +} __packed;
> +#else
> +#error "Bitfield order not defined!"
> +#endif
> +
>  struct mpi_msg_hdr {
> __le32  header; /* Bits [11:0] - Message operation code */
> /* Bits [15:12] - Message Category */
> @@ -248,7 +344,7 @@ struct mpi_msg_hdr {
>  struct phy_start_req {
> __le32  tag;
> __le32  ase_sh_lm_slr_phyid;
> -   struct sas_identify_frame sas_identify; /* 28 Bytes */
> +   struct sas_identify_frame_local sas_identify; /* 28 Bytes */
> __le32 spasti;
> u32 reserved[21];
>  } __attribute__((packed, aligned(4)));
> --
> 2.12.3
>
Thanks,
Acked-by: Jack Wang 

-- 
Jack Wang
Linux Kernel Developer

ProfitBricks GmbH
Greifswalder Str. 207
D - 10405 Berlin

Tel:   +49 30 577 008  042
Fax:  +49 30 577 008 299
Email:jinpu.w...@profitbricks.com
URL:  https://www.profitbricks.de

Sitz der Gesellschaft: Berlin
Registergericht: Amtsgericht Charlottenburg, HRB 125506 B
Geschäftsführer: Achim Weiss

Re: scsi: mvsas: replace kfree with scsi_host_put

[Jinpu Wang]

On Tue, Aug 8, 2017 at 2:02 PM, Pan Bian  wrote:
> The return value of scsi_host_alloc() should be released by
> scsi_host_put(). However, in function mvs_pci_init(), kfree()
> is used. This patch replaces kfree() with scsi_host_put() to avoid
> possible memory leaks.
>
> Signed-off-by: Pan Bian 
> ---
>  drivers/scsi/mvsas/mv_init.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/scsi/mvsas/mv_init.c b/drivers/scsi/mvsas/mv_init.c
> index 4e047b5..75bdb38 100644
> --- a/drivers/scsi/mvsas/mv_init.c
> +++ b/drivers/scsi/mvsas/mv_init.c
> @@ -557,14 +557,14 @@ static int mvs_pci_init(struct pci_dev *pdev, const 
> struct pci_device_id *ent)
> SHOST_TO_SAS_HA(shost) =
> kcalloc(1, sizeof(struct sas_ha_struct), GFP_KERNEL);
> if (!SHOST_TO_SAS_HA(shost)) {
> -   kfree(shost);
> +   scsi_host_put(shost);
> rc = -ENOMEM;
> goto err_out_regions;
> }
>
> rc = mvs_prep_sas_ha_init(shost, chip);
> if (rc) {
> -   kfree(shost);
> +   scsi_host_put(shost);
> rc = -ENOMEM;
> goto err_out_regions;
> }
> --
> 1.9.1
>
>

Looks good to me! Thanks Pan!

Reviewed-by: Jack Wang 

-- 
Jack Wang
Linux Kernel Developer

ProfitBricks GmbH
Greifswalder Str. 207
D - 10405 Berlin

Re: scsi: pm8001: fix double free in pm8001_pci_probe

[Jinpu Wang]

On Tue, Aug 8, 2017 at 1:40 PM, Pan Bian  wrote:
> In function pm8001_pci_probe(), on errors that the control flow jumps to
> label err_out_ha_free, function pm8001_free() is called. In pm8001_free(),
> scsi_host_put() is called to release shost, which keeps the return value
> of scsi_host_alloc(). After pm8001_free() returns, kfree() is called to
> free shost again, resulting in a double free bug. This patch removes
> scsi_host_put() from pm8001_free() and explicitly calls scsi_host_put()
> to release Scsi_Host in need.
>
> Signed-off-by: Pan Bian 
Thanks Pan!

Looks good to me!

Reviewed-by: Jack Wang 

> ---
>  drivers/scsi/pm8001/pm8001_init.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/scsi/pm8001/pm8001_init.c 
> b/drivers/scsi/pm8001/pm8001_init.c
> index 034b2f7..2908881 100644
> --- a/drivers/scsi/pm8001/pm8001_init.c
> +++ b/drivers/scsi/pm8001/pm8001_init.c
> @@ -160,8 +160,6 @@ static void pm8001_free(struct pm8001_hba_info *pm8001_ha)
> }
> }
> PM8001_CHIP_DISP->chip_iounmap(pm8001_ha);
> -   if (pm8001_ha->shost)
> -   scsi_host_put(pm8001_ha->shost);
> flush_workqueue(pm8001_wq);
> kfree(pm8001_ha->tags);
> kfree(pm8001_ha);
> @@ -1073,7 +1071,7 @@ static int pm8001_pci_probe(struct pci_dev *pdev,
>  err_out_free:
> kfree(SHOST_TO_SAS_HA(shost));
>  err_out_free_host:
> -   kfree(shost);
> +   scsi_host_put(shost);
>  err_out_regions:
> pci_release_regions(pdev);
>  err_out_disable:
> @@ -1112,6 +1110,7 @@ static void pm8001_pci_remove(struct pci_dev *pdev)
> for (j = 0; j < PM8001_MAX_MSIX_VEC; j++)
> tasklet_kill(_ha->tasklet[j]);
>  #endif
> +   scsi_host_put(pm8001_ha->shost);
> pm8001_free(pm8001_ha);
> kfree(sha->sas_phy);
> kfree(sha->sas_port);
> --
> 1.9.1
>
>



-- 
Jack Wang
Linux Kernel Developer

ProfitBricks GmbH
Greifswalder Str. 207
D - 10405 Berlin

Tel:   +49 30 577 008  042
Fax:  +49 30 577 008 299
Email:jinpu.w...@profitbricks.com
URL:  https://www.profitbricks.de

Sitz der Gesellschaft: Berlin
Registergericht: Amtsgericht Charlottenburg, HRB 125506 B
Geschäftsführer: Achim Weiss

Re: [PATCH 04/29] scsi: pm8001: constify pci_device_id.

[Jinpu Wang]

On Sun, Jul 30, 2017 at 10:37 AM, Arvind Yadav
 wrote:
> pci_device_id are not supposed to change at runtime. All functions
> working with pci_device_id provided by  work with
> const pci_device_id. So mark the non-const structs as const.
>
> Signed-off-by: Arvind Yadav 
> ---
>  drivers/scsi/pm8001/pm8001_init.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/pm8001/pm8001_init.c 
> b/drivers/scsi/pm8001/pm8001_init.c
> index 034b2f7..f2757cc 100644
> --- a/drivers/scsi/pm8001/pm8001_init.c
> +++ b/drivers/scsi/pm8001/pm8001_init.c
> @@ -1270,7 +1270,7 @@ static int pm8001_pci_resume(struct pci_dev *pdev)
>  /* update of pci device, vendor id and driver data with
>   * unique value for each of the controller
>   */
> -static struct pci_device_id pm8001_pci_table[] = {
> +static const struct pci_device_id pm8001_pci_table[] = {
> { PCI_VDEVICE(PMC_Sierra, 0x8001), chip_8001 },
> { PCI_VDEVICE(PMC_Sierra, 0x8006), chip_8006 },
> { PCI_VDEVICE(ADAPTEC2, 0x8006), chip_8006 },
> --
> 2.7.4
>

Thanks,
Acked-by: Jack Wang 

-- 
Jack Wang
Linux Kernel Developer

Re: [PATCH] scsi: sas: move scsi_remove_host call into sas_remove_host

[Jinpu Wang]

On Fri, Apr 21, 2017 at 2:11 PM, Johannes Thumshirn <jthumsh...@suse.de> wrote:
> Move scsi_remove_host call into sas_remove_host and remove it from SAS HBA
> drivers, so we don't mess up the ordering. This solves an issue with double
> deleting sysfs entries that was introduced by the change of sysfs behaviour
> from commit bcdde7e ("sysfs: make __sysfs_remove_dir() recursive").
>
> Signed-off-by: Johannes Thumshirn <jthumsh...@suse.de>
> Suggested-by: Christoph Hellwig <h...@lst.de>
> Cc: Hannes Reinecke <h...@suse.de>
> Cc: James Bottomley <j...@linux.vnet.ibm.com>
> Cc: Jinpu Wang <jinpu.w...@profitbricks.com>
> Cc: John Garry <john.ga...@huawei.com>
> ---
>  drivers/scsi/aic94xx/aic94xx_init.c   | 1 -
>  drivers/scsi/hisi_sas/hisi_sas_main.c | 1 -
>  drivers/scsi/isci/init.c  | 1 -
>  drivers/scsi/mpt3sas/mpt3sas_scsih.c  | 1 -
>  drivers/scsi/mvsas/mv_init.c  | 1 -
>  drivers/scsi/pm8001/pm8001_init.c | 1 -
>  drivers/scsi/scsi_transport_sas.c | 8 ++--
>  7 files changed, 6 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/scsi/aic94xx/aic94xx_init.c 
> b/drivers/scsi/aic94xx/aic94xx_init.c
> index 662b2321d1b0..a14ba7a6b81e 100644
> --- a/drivers/scsi/aic94xx/aic94xx_init.c
> +++ b/drivers/scsi/aic94xx/aic94xx_init.c
> @@ -703,7 +703,6 @@ static int asd_unregister_sas_ha(struct asd_ha_struct 
> *asd_ha)
>  {
> int err;
>
> -   scsi_remove_host(asd_ha->sas_ha.core.shost);
> err = sas_unregister_ha(_ha->sas_ha);
>
> sas_remove_host(asd_ha->sas_ha.core.shost);
> diff --git a/drivers/scsi/hisi_sas/hisi_sas_main.c 
> b/drivers/scsi/hisi_sas/hisi_sas_main.c
> index 53637a941b94..843bedae6c09 100644
> --- a/drivers/scsi/hisi_sas/hisi_sas_main.c
> +++ b/drivers/scsi/hisi_sas/hisi_sas_main.c
> @@ -1583,7 +1583,6 @@ int hisi_sas_remove(struct platform_device *pdev)
> struct hisi_hba *hisi_hba = sha->lldd_ha;
> struct Scsi_Host *shost = sha->core.shost;
>
> -   scsi_remove_host(sha->core.shost);
> sas_unregister_ha(sha);
> sas_remove_host(sha->core.shost);
>
> diff --git a/drivers/scsi/isci/init.c b/drivers/scsi/isci/init.c
> index 0b5b5db0d0f8..45371179ab87 100644
> --- a/drivers/scsi/isci/init.c
> +++ b/drivers/scsi/isci/init.c
> @@ -272,7 +272,6 @@ static void isci_unregister(struct isci_host *isci_host)
> return;
>
> shost = to_shost(isci_host);
> -   scsi_remove_host(shost);
> sas_unregister_ha(_host->sas_ha);
>
> sas_remove_host(shost);
> diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c 
> b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
> index 919ba2bb15f1..a5d872664257 100644
> --- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
> +++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
> @@ -8283,7 +8283,6 @@ static void scsih_remove(struct pci_dev *pdev)
> }
>
> sas_remove_host(shost);
> -   scsi_remove_host(shost);
> mpt3sas_base_detach(ioc);
> spin_lock(_lock);
> list_del(>list);
> diff --git a/drivers/scsi/mvsas/mv_init.c b/drivers/scsi/mvsas/mv_init.c
> index 8280046fd1f0..4e047b5001a6 100644
> --- a/drivers/scsi/mvsas/mv_init.c
> +++ b/drivers/scsi/mvsas/mv_init.c
> @@ -642,7 +642,6 @@ static void mvs_pci_remove(struct pci_dev *pdev)
> tasklet_kill(&((struct mvs_prv_info *)sha->lldd_ha)->mv_tasklet);
>  #endif
>
> -   scsi_remove_host(mvi->shost);
> sas_unregister_ha(sha);
> sas_remove_host(mvi->shost);
>
> diff --git a/drivers/scsi/pm8001/pm8001_init.c 
> b/drivers/scsi/pm8001/pm8001_init.c
> index 417368ccb686..034b2f7d1135 100644
> --- a/drivers/scsi/pm8001/pm8001_init.c
> +++ b/drivers/scsi/pm8001/pm8001_init.c
> @@ -1088,7 +1088,6 @@ static void pm8001_pci_remove(struct pci_dev *pdev)
> struct pm8001_hba_info *pm8001_ha;
> int i, j;
> pm8001_ha = sha->lldd_ha;
> -   scsi_remove_host(pm8001_ha->shost);
> sas_unregister_ha(sha);
> sas_remove_host(pm8001_ha->shost);
> list_del(_ha->list);
> diff --git a/drivers/scsi/scsi_transport_sas.c 
> b/drivers/scsi/scsi_transport_sas.c
> index cdbb293aca08..ca0e5a9a17f8 100644
> --- a/drivers/scsi/scsi_transport_sas.c
> +++ b/drivers/scsi/scsi_transport_sas.c
> @@ -370,12 +370,16 @@ EXPORT_SYMBOL(sas_remove_children);
>   * sas_remove_host  -  tear down a Scsi_Host's SAS data structures
>   * @shost: Scsi Host that is torn down
>   *
> - * Removes all SAS PHYs and remote PHYs for a given Scsi_Host.
> - * Must be called just before scsi_remove_host for SAS HBAs.
> + * Removes all 

Re: [PATCH 1/2] scsi: sas: flush destruct workqueue on device unregister

[Jinpu Wang]

On Wed, Mar 29, 2017 at 2:26 PM, Johannes Thumshirn  wrote:
> On Wed, Mar 29, 2017 at 12:53:28PM +0100, John Garry wrote:
>> On 29/03/2017 12:29, Johannes Thumshirn wrote:
>> >On Wed, Mar 29, 2017 at 12:15:44PM +0100, John Garry wrote:
>> >>On 29/03/2017 10:41, Johannes Thumshirn wrote:
>> >>>In the advent of an SAS device unregister we have to wait for all destruct
>> >>>works to be done to not accidently delay deletion of a SAS rphy or it's
>> >>>children to the point when we're removing the SCSI or SAS hosts.
>> >>>
>> >>>Signed-off-by: Johannes Thumshirn 
>> >>>---
>> >>>drivers/scsi/libsas/sas_discover.c | 4 
>> >>>1 file changed, 4 insertions(+)
>> >>>
>> >>>diff --git a/drivers/scsi/libsas/sas_discover.c 
>> >>>b/drivers/scsi/libsas/sas_discover.c
>> >>>index 60de662..75b18f1 100644
>> >>>--- a/drivers/scsi/libsas/sas_discover.c
>> >>>+++ b/drivers/scsi/libsas/sas_discover.c
>> >>>@@ -382,9 +382,13 @@ void sas_unregister_dev(struct asd_sas_port *port, 
>> >>>struct domain_device *dev)
>> >>>   }
>> >>>
>> >>>   if (!test_and_set_bit(SAS_DEV_DESTROY, >state)) {
>> >>>+  struct sas_discovery *disc = >port->disc;
>> >>>+  struct sas_work *sw = >disc_work[DISCE_DESTRUCT].work;
>> >>>+
>> >>>   sas_rphy_unlink(dev->rphy);
>> >>>   list_move_tail(>disco_list_node, >destroy_list);
>> >>>   sas_discover_event(dev->port, DISCE_DESTRUCT);
>> >>>+  flush_work(>work);
>> >>
>> >>I quickly tested plugging out the expander and we never get past this call
>> >>to flush - a hang results:
>> >
>> >Can you activat lockdep so we can see which lock it is that we're blocking 
>> >on?
>> >
>>
>> I have it on:
>> CONFIG_LOCKDEP_SUPPORT=y
>> CONFIG_LOCKD=y
>> CONFIG_LOCKD_V4=y
>>
>> >It's most likely in sas_unregister_common_dev() but this function takes two 
>> >spin
>> >locks, port->dev_list_lock and ha->lock.
>> >
>>
>> We can see from the callstack I provided that we're working in workqueue
>> scsi_wq_0 and trying to flush that same queue.
>
> Aaahh, now I get what's happening (with some kicks^Whelp from Hannes I admit).
>
> The sas_unregister_dev() comes from the work queued by notify_phy_event(). So 
> this patch must be
> replaced by (untested):
>
> diff --git a/drivers/scsi/scsi_transport_sas.c 
> b/drivers/scsi/scsi_transport_sas.c
> index cdbb293..e1e6492 100644
> --- a/drivers/scsi/scsi_transport_sas.c
> +++ b/drivers/scsi/scsi_transport_sas.c
> @@ -375,6 +375,7 @@ void sas_remove_children(struct device *dev)
>   */
>  void sas_remove_host(struct Scsi_Host *shost)
>  {
> +   scsi_flush_work(shost);
> sas_remove_children(>shost_gendev);
>  }
>  EXPORT_SYMBOL(sas_remove_host);
>
> John, mind giving that one a shot in your test setup as well?
>
> Thanks,
> Johannes
>
> --
> Johannes Thumshirn  Storage
> jthumsh...@suse.de+49 911 74053 689
> SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
> GF: Felix Imendörffer, Jane Smithard, Graham Norton
> HRB 21284 (AG Nürnberg)
> Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

Haha, I have same idea :)
Have no test env, so if John could test it, it will be great.
-- 
Jack Wang
Linux Kernel Developer

ProfitBricks GmbH
Greifswalder Str. 207
D - 10405 Berlin

Tel:   +49 30 577 008  042
Fax:  +49 30 577 008 299
Email:jinpu.w...@profitbricks.com
URL:  https://www.profitbricks.de

Sitz der Gesellschaft: Berlin
Registergericht: Amtsgericht Charlottenburg, HRB 125506 B
Geschäftsführer: Achim Weiss

Re: [PATCH 6/6] scsi: pm8001: remove the SAS host after the SCSI host

[Jinpu Wang]

On Wed, Mar 29, 2017 at 11:41 AM, Johannes Thumshirn  wrote:
> After commit bcdde7e ("sysfs: make __sysfs_remove_dir() recursive") changed 
> the
> removal path of kernfs to make it recursive we have to remove the SAS host
> before the SCSI host or we will see sysfs warnings on not found sysfs groups 
> for
> kobjects.
>
> Signed-off-by: Johannes Thumshirn 
> ---
>  drivers/scsi/pm8001/pm8001_init.c | 14 --
>  1 file changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/scsi/pm8001/pm8001_init.c 
> b/drivers/scsi/pm8001/pm8001_init.c
> index 417368c..9116e9c 100644
> --- a/drivers/scsi/pm8001/pm8001_init.c
> +++ b/drivers/scsi/pm8001/pm8001_init.c
> @@ -1086,11 +1086,21 @@ static void pm8001_pci_remove(struct pci_dev *pdev)
>  {
> struct sas_ha_struct *sha = pci_get_drvdata(pdev);
> struct pm8001_hba_info *pm8001_ha;
> +   unsigned long flags;
> +   struct Scsi_Host *shost;
> int i, j;
> pm8001_ha = sha->lldd_ha;
> -   scsi_remove_host(pm8001_ha->shost);
> +   shost = pm8001_ha->shost;
> +
> +   spin_lock_irqsave(shost->host_lock, flags);
> +   if (scsi_host_set_state(shost, SHOST_CANCEL))
> +   WARN_ON(scsi_host_set_state(shost, SHOST_CANCEL_RECOVERY));
> +   spin_unlock_irqrestore(shost->host_lock, flags);
> +
> sas_unregister_ha(sha);
> -   sas_remove_host(pm8001_ha->shost);
> +   sas_remove_host(shost);
> +   scsi_remove_host(shost);
> +
> list_del(_ha->list);
> PM8001_CHIP_DISP->interrupt_disable(pm8001_ha, 0xFF);
> PM8001_CHIP_DISP->chip_soft_rst(pm8001_ha);
> --
> 1.8.5.6
>
Thanks Johannes for taking care of this. Looks good to me,

I have a question regarding the scsi_host_set_state change, why do we need that?
Can't we simply change the order of sas_remove_host and scsi_remove_host?

Cheers?
-- 
Jack Wang

Re: [PATCH] scsi: pm8001: build in relevant functions and code on PM8001_USE_MSIX

[Jinpu Wang]

On Wed, Mar 22, 2017 at 7:50 PM, Colin King  wrote:
> From: Colin Ian King 
>
> Currently the misx and intx variables of the interrupt enable/disable
> helper functions are built in no matter what the setting of the
> macro PM8001_USE_MSIX.  Clean this up by just building in the
> necessary helper functions and calls to these functions depending on
> the setting of PM8001_USE_MSIX.  This addresses several dead code
> paths found by static analysis with CoverityScan.
>
> Signed-off-by: Colin Ian King 
> ---
>  drivers/scsi/pm8001/pm8001_hwi.c | 63 
> +---
>  1 file changed, 33 insertions(+), 30 deletions(-)

Thanks Colin,

Mind to also change pm80xx_hw.c?

-- 
Jack Wang
Linux Kernel Developer

ProfitBricks GmbH
Greifswalder Str. 207
D - 10405 Berlin

Tel:   +49 30 577 008  042
Fax:  +49 30 577 008 299
Email:jinpu.w...@profitbricks.com
URL:  https://www.profitbricks.de

Sitz der Gesellschaft: Berlin
Registergericht: Amtsgericht Charlottenburg, HRB 125506 B
Geschäftsführer: Achim Weiss

Re: [PATCH] scsi: pm8001: build in relevant functions and code on PM8001_USE_MSIX

[Jinpu Wang]

On Thu, Mar 23, 2017 at 9:34 AM, walter harms <wha...@bfs.de> wrote:

>> @@ -4613,15 +4616,15 @@ static int pm8001_chip_phy_ctl_req(struct 
>> pm8001_hba_info *pm8001_ha,
>>
>>  static u32 pm8001_chip_is_our_interupt(struct pm8001_hba_info *pm8001_ha)
>>  {
>> - u32 value;
>>  #ifdef PM8001_USE_MSIX
>>   return 1;
>> -#endif
>> - value = pm8001_cr32(pm8001_ha, 0, MSGU_ODR);
>> +#else
>> + u32 value = pm8001_cr32(pm8001_ha, 0, MSGU_ODR);
>> +
>>   if (value)
>>   return 1;
>>   return 0;
>> -
>> +#endif
>>  }
>>
>
> This is a bit strange, why do this function return u32 ?
>
> re,
>  wh
>
>>  /**

Hi Walter,

Feel free to submit a patch to change it to bool.

Thanks

-- 
Jinpu Wang
Linux Kernel Developer

ProfitBricks GmbH
Greifswalder Str. 207
D - 10405 Berlin

Tel:   +49 30 577 008  042
Fax:  +49 30 577 008 299
Email:jinpu.w...@profitbricks.com
URL:  https://www.profitbricks.de

Sitz der Gesellschaft: Berlin
Registergericht: Amtsgericht Charlottenburg, HRB 125506 B
Geschäftsführer: Achim Weiss

Re: Question about latest PM80xx driver

[Jinpu Wang]

Hi Duc,

On Thu, Nov 3, 2016 at 2:02 AM, Duc Dang  wrote:
> Hi Jack, Lindar and All,
>
> I was testing Adaptec PMC-Sierra PM8018 SAS HBA [Series 7H] card
> [9005:8088] with our ARM64 server SoC and occasionally saw a hang in
> pm8001_pci_probe. It looks like the driver was trying to read nvmd
> information (using get_nvmd_req) and wait for completion but the
> completion never happens, so I got a kernel timeout trace:
>
> "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
> swapper/0   D ffc86ec0 0 1  0 0x
> Call trace:
> [] __switch_to+0x90/0xb0
> [] __schedule+0x204/0x670
> [] schedule+0x40/0xb0
> [] schedule_timeout+0x15c/0x1b0
> [] wait_for_common+0xa0/0x150
> [] wait_for_completion+0x14/0x20
> [] pm8001_pci_probe+0xb7c/0xd10
> [] pci_device_probe+0xa0/0x120
> [] driver_probe_device+0x12c/0x2e0
> [] __driver_attach+0x9c/0xa0
> [] bus_for_each_dev+0x5c/0xa0
> [] driver_attach+0x20/0x30
> [] bus_add_driver+0x108/0x230
> [] driver_register+0x60/0x100
> [] __pci_register_driver+0x40/0x50
> [] pm8001_init+0x70/0xa8
> [] do_one_initcall+0x94/0x1b0
> [] kernel_init_freeable+0x150/0x1f4
> [] kernel_init+0x10/0xf0
>
> The current driver in kernel.org is 0.1.38, is this already the latest one?
>
> I saw in the Readme file of BIOS package in Microsemi website
> (http://storage.microsemi.com/en-us/downloads/bios_fw/bios_fw_ver/productid=asa-71605h=adaptec+71605h.php)
> mentions below issue. Is the hang symptom similar to the one that I
> saw?
>
> 5.14 FreeBSD Hangs when No Devices on HBA
> On FreeBSD systems, the OS fails to boot when no devices are
> connected to the HBAs.
>
> WORKAROUND: Connect at least one device to the HBA to complete
> the boot process.
>
>
> Regards,
> Duc Dang.

Likely it related to your hardware, maybe you don't have NVM device to
save VPD related info, you can tweak that part, eg comment out
#define PM8001_READ_VPD.



-- 
Mit freundlichen Grüßen,
Best Regards,

Jack Wang

Linux Kernel Developer Storage
ProfitBricks GmbH  The IaaS-Company.
--
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] scsi: squash lines for simple wrapper functions

[Jinpu Wang]

On Wed, Sep 7, 2016 at 12:38 AM, Masahiro Yamada
 wrote:
> Remove unneeded variables and assignments.
>
> Signed-off-by: Masahiro Yamada 
> ---
>
>  drivers/scsi/aic7xxx/aic79xx_osm.c  |  6 +-
>  drivers/scsi/arcmsr/arcmsr_hba.c|  4 +---
>  drivers/scsi/esas2r/esas2r_ioctl.c  | 20 
>  drivers/scsi/lpfc/lpfc_attr.c   |  8 ++--
>  drivers/scsi/lpfc/lpfc_ct.c |  6 +-
>  drivers/scsi/lpfc/lpfc_scsi.c   |  5 +
>  drivers/scsi/lpfc/lpfc_sli.c|  5 +
>  drivers/scsi/megaraid/megaraid_mm.c |  6 +-
>  drivers/scsi/mpt3sas/mpt3sas_ctl.c  | 30 ++
>  drivers/scsi/osd/osd_initiator.c|  5 +
>  drivers/scsi/pm8001/pm8001_ctl.c| 10 ++
>  drivers/scsi/sg.c   | 10 +++---
>  12 files changed, 32 insertions(+), 83 deletions(-)
>
> diff --git a/drivers/scsi/aic7xxx/aic79xx_osm.c 
> b/drivers/scsi/aic7xxx/aic79xx_osm.c
> index 2588b8f..1169e85 100644
> --- a/drivers/scsi/aic7xxx/aic79xx_osm.c
> +++ b/drivers/scsi/aic7xxx/aic79xx_osm.c
> @@ -767,11 +767,7 @@ ahd_linux_biosparam(struct scsi_device *sdev, struct 
> block_device *bdev,
>  static int
>  ahd_linux_abort(struct scsi_cmnd *cmd)
>  {
> -   int error;
> -
> -   error = ahd_linux_queue_abort_cmd(cmd);
> -
> -   return error;
> +   return ahd_linux_queue_abort_cmd(cmd);
>  }
>
>  /*
> diff --git a/drivers/scsi/arcmsr/arcmsr_hba.c 
> b/drivers/scsi/arcmsr/arcmsr_hba.c
> index 7640498..f1c86f5 100644
> --- a/drivers/scsi/arcmsr/arcmsr_hba.c
> +++ b/drivers/scsi/arcmsr/arcmsr_hba.c
> @@ -3946,9 +3946,7 @@ sleep:
>  static int arcmsr_abort_one_cmd(struct AdapterControlBlock *acb,
> struct CommandControlBlock *ccb)
>  {
> -   int rtn;
> -   rtn = arcmsr_polling_ccbdone(acb, ccb);
> -   return rtn;
> +   return arcmsr_polling_ccbdone(acb, ccb);
>  }
>
>  static int arcmsr_abort(struct scsi_cmnd *cmd)
> diff --git a/drivers/scsi/esas2r/esas2r_ioctl.c 
> b/drivers/scsi/esas2r/esas2r_ioctl.c
> index 3e84834..12c6284 100644
> --- a/drivers/scsi/esas2r/esas2r_ioctl.c
> +++ b/drivers/scsi/esas2r/esas2r_ioctl.c
> @@ -373,18 +373,14 @@ static bool csmi_ioctl_tunnel(struct esas2r_adapter *a,
>
>  static bool check_lun(struct scsi_lun lun)
>  {
> -   bool result;
> -
> -   result = ((lun.scsi_lun[7] == 0) &&
> - (lun.scsi_lun[6] == 0) &&
> - (lun.scsi_lun[5] == 0) &&
> - (lun.scsi_lun[4] == 0) &&
> - (lun.scsi_lun[3] == 0) &&
> - (lun.scsi_lun[2] == 0) &&
> -/* Byte 1 is intentionally skipped */
> - (lun.scsi_lun[0] == 0));
> -
> -   return result;
> +   return (lun.scsi_lun[7] == 0) &&
> +  (lun.scsi_lun[6] == 0) &&
> +  (lun.scsi_lun[5] == 0) &&
> +  (lun.scsi_lun[4] == 0) &&
> +  (lun.scsi_lun[3] == 0) &&
> +  (lun.scsi_lun[2] == 0) &&
> +   /* Byte 1 is intentionally skipped */
> +  (lun.scsi_lun[0] == 0);
>  }
>
>  static int csmi_ioctl_callback(struct esas2r_adapter *a,
> diff --git a/drivers/scsi/lpfc/lpfc_attr.c b/drivers/scsi/lpfc/lpfc_attr.c
> index f101990..79cb019 100644
> --- a/drivers/scsi/lpfc/lpfc_attr.c
> +++ b/drivers/scsi/lpfc/lpfc_attr.c
> @@ -2625,12 +2625,8 @@ lpfc_oas_lun_state_change(struct lpfc_hba *phba, 
> uint8_t vpt_wwpn[],
>   uint8_t tgt_wwpn[], uint64_t lun,
>   uint32_t oas_state, uint8_t pri)
>  {
> -
> -   int rc;
> -
> -   rc = lpfc_oas_lun_state_set(phba, vpt_wwpn, tgt_wwpn, lun,
> -   oas_state, pri);
> -   return rc;
> +   return lpfc_oas_lun_state_set(phba, vpt_wwpn, tgt_wwpn, lun,
> + oas_state, pri);
>  }
>
>  /**
> diff --git a/drivers/scsi/lpfc/lpfc_ct.c b/drivers/scsi/lpfc/lpfc_ct.c
> index 63e48d4..383943d 100644
> --- a/drivers/scsi/lpfc/lpfc_ct.c
> +++ b/drivers/scsi/lpfc/lpfc_ct.c
> @@ -188,12 +188,8 @@ lpfc_ct_unsol_event(struct lpfc_hba *phba, struct 
> lpfc_sli_ring *pring,
>  int
>  lpfc_ct_handle_unsol_abort(struct lpfc_hba *phba, struct hbq_dmabuf *dmabuf)
>  {
> -   int handled;
> -
> /* CT upper level goes through BSG */
> -   handled = lpfc_bsg_ct_unsol_abort(phba, dmabuf);
> -
> -   return handled;
> +   return lpfc_bsg_ct_unsol_abort(phba, dmabuf);
>  }
>
>  static void
> diff --git a/drivers/scsi/lpfc/lpfc_scsi.c b/drivers/scsi/lpfc/lpfc_scsi.c
> index d197aa1..59188b6 100644
> --- a/drivers/scsi/lpfc/lpfc_scsi.c
> +++ b/drivers/scsi/lpfc/lpfc_scsi.c
> @@ -2857,10 +2857,7 @@ lpfc_bg_crc(uint8_t *data, int count)
>  static uint16_t
>  lpfc_bg_csum(uint8_t *data, int count)
>  {
> -   uint16_t ret;
> -
> -   ret = ip_compute_csum(data, count);
> -   return ret;
> +   return ip_compute_csum(data, count);
>  }
>
>  /*
> diff 

Re: [PATCH] SCSI-aic94xx: Delete unnecessary checks before the function call "kmem_cache_destroy"

[Jinpu Wang]

On Sun, Jul 24, 2016 at 1:51 PM, SF Markus Elfring
 wrote:
>
> > From: Markus Elfring 
> > Date: Tue, 17 Nov 2015 08:14:52 +0100
> >
> > The kmem_cache_destroy() function tests whether its argument is NULL
> > and then returns immediately. Thus the test around the calls is not needed.
> >
> > This issue was detected by using the Coccinelle software.
> >
> > Signed-off-by: Markus Elfring 
> > ---
> >  drivers/scsi/aic94xx/aic94xx_init.c | 7 ++-
> >  1 file changed, 2 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/scsi/aic94xx/aic94xx_init.c 
> > b/drivers/scsi/aic94xx/aic94xx_init.c
> > index 662b232..ab93049 100644
> > --- a/drivers/scsi/aic94xx/aic94xx_init.c
> > +++ b/drivers/scsi/aic94xx/aic94xx_init.c
> > @@ -660,12 +660,9 @@ Err:
> >
> >  static void asd_destroy_global_caches(void)
> >  {
> > - if (asd_dma_token_cache)
> > - kmem_cache_destroy(asd_dma_token_cache);
> > + kmem_cache_destroy(asd_dma_token_cache);
> >   asd_dma_token_cache = NULL;
> > -
> > - if (asd_ascb_cache)
> > - kmem_cache_destroy(asd_ascb_cache);
> > + kmem_cache_destroy(asd_ascb_cache);
> >   asd_ascb_cache = NULL;
> >  }
> >
> >
>
> How do you think about to integrate this update suggestion
> into another source code repository?
>
> Regards,
> Markus


Looks good to me!
Reviewed-by: Jack Wang 

PS: resend to fix mal-format rejection, sorry!
-- 

Mit freundlichen Grüßen,
Best Regards,

Jack Wang
--
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 5/7] pm8001: fix typo

[Jinpu Wang]

On Tue, May 17, 2016 at 4:38 PM, Julia Lawall  wrote:
> firmare -> firmware
>
> Signed-off-by: Julia Lawall 
>
> ---
>  drivers/scsi/pm8001/pm8001_init.c |2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/pm8001/pm8001_init.c 
> b/drivers/scsi/pm8001/pm8001_init.c
> index 6bd7bf4..fdbee8b4 100644
> --- a/drivers/scsi/pm8001/pm8001_init.c
> +++ b/drivers/scsi/pm8001/pm8001_init.c
> @@ -1249,7 +1249,7 @@ static int pm8001_pci_resume(struct pci_dev *pdev)
>
> /* Chip documentation for the 8070 and 8072 SPCv*/
> /* states that a 500ms minimum delay is required*/
> -   /* before issuing commands.  Otherwise, the firmare */
> +   /* before issuing commands. Otherwise, the firmware */
> /* will enter an unrecoverable state.   */
>
> if (pm8001_ha->chip_id == chip_8070 ||
>

Thanks,
Acked-by: Jack Wang 

-- 
Mit freundlichen Grüßen,
Best Regards,

Jack Wang
--
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] scsi_lib: correctly retry failed zero length REQ_TYPE_FS commands

[Jinpu Wang]

On Tue, May 17, 2016 at 11:28 AM, Johannes Thumshirn  wrote:
> On Fri, May 13, 2016 at 12:04:06PM -0700, James Bottomley wrote:
>> When SCSI was written, all commands coming from the filesystem
>> (REQ_TYPE_FS commands) had data.  This meant that our signal for
>> needing to complete the command was the number of bytes completed being
>> equal to the number of bytes in the request.  Unfortunately, with the
>> advent of flush barriers, we can now get zero length REQ_TYPE_FS
>> commands, which confuse this logic because they satisfy the condition
>> every time.  This means they never get retried even for retryable
>> conditions, like UNIT ATTENTION because we complete them early assuming
>> they're done.  Fix this by special casing the early completion
>> condition to recognise zero length commands with errors and let them
>> drop through to the retry code.
>>
>> Reported-by: Sebastian Parschauer 
>> Signed-off-by: James E.J. Bottomley 
>>
>> ---
>>
>> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
>> index 8106515..f704d02 100644
>> --- a/drivers/scsi/scsi_lib.c
>> +++ b/drivers/scsi/scsi_lib.c
>> @@ -911,9 +911,12 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned 
>> int good_bytes)
>>   }
>>
>>   /*
>> -  * If we finished all bytes in the request we are done now.
>> +  * special case: failed zero length commands always need to
>> +  * drop down into the retry code. Otherwise, if we finished
>> +  * all bytes in the request we are done now.
>>*/
>> - if (!scsi_end_request(req, error, good_bytes, 0))
>> + if (!(blk_rq_bytes(req) == 0 && error) &&
>> + !scsi_end_request(req, error, good_bytes, 0))
>>   return;
>
>
> Naive question, why aren't we checking for blk_rq_bytes(req) == 0 && error in
> scsi_end_request()?
>

Hi Johannes,

Hannes also suggested this way, but that lead to BUG see:
http://www.spinics.net/lists/linux-scsi/msg96908.html


-- 
Mit freundlichen Grüßen,
Best Regards,

Jack Wang

Linux Kernel Developer Storage
ProfitBricks GmbH  The IaaS-Company.
--
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] scsi_lib: correctly retry failed zero length REQ_TYPE_FS commands

[Jinpu Wang]

On Fri, May 13, 2016 at 9:04 PM, James Bottomley
 wrote:
> When SCSI was written, all commands coming from the filesystem
> (REQ_TYPE_FS commands) had data.  This meant that our signal for
> needing to complete the command was the number of bytes completed being
> equal to the number of bytes in the request.  Unfortunately, with the
> advent of flush barriers, we can now get zero length REQ_TYPE_FS
> commands, which confuse this logic because they satisfy the condition
> every time.  This means they never get retried even for retryable
> conditions, like UNIT ATTENTION because we complete them early assuming
> they're done.  Fix this by special casing the early completion
> condition to recognise zero length commands with errors and let them
> drop through to the retry code.
>
> Reported-by: Sebastian Parschauer 
> Signed-off-by: James E.J. Bottomley 
>
> ---
>
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index 8106515..f704d02 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -911,9 +911,12 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned 
> int good_bytes)
> }
>
> /*
> -* If we finished all bytes in the request we are done now.
> +* special case: failed zero length commands always need to
> +* drop down into the retry code. Otherwise, if we finished
> +* all bytes in the request we are done now.
>  */
> -   if (!scsi_end_request(req, error, good_bytes, 0))
> +   if (!(blk_rq_bytes(req) == 0 && error) &&
> +   !scsi_end_request(req, error, good_bytes, 0))
> return;
>
> /*

Hi James,

Thanks for your new patch. I tested on my environment, works fine.
You can add:
Tested-by: Jack Wang 
-- 
Mit freundlichen Grüßen,
Best Regards,

Jack Wang

Linux Kernel Developer Storage
ProfitBricks GmbH  The IaaS-Company.
--
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

[PATCHv3]scsi: don't fail zero length request too early

[Jinpu Wang]

Hi James, and all,

I guess you're busy on other staff, so I create patch below as you
suggested, I think we also need this into stable.

>From 99eab170653544fa1e1bc9511ec055ba70e183d2 Mon Sep 17 00:00:00 2001
From: Jack Wang 
Date: Fri, 13 May 2016 09:53:21 +0200
Subject: [PATCH] scsi: don't fail zero length request too early

We hit IO error in our production when SYNC on multipath devices during resize
device on target side, the problem turns out scsi driver passes up as IO
error when sense data is UNIT_ATTENTION and ASC && ASCQ indicate
Capacity data has changed, even storage side sync the data properly.

Dig it further turns out we need special case on zero length commands
(currently only FLUSH), when it fails, we always need to drop down
into retry code.

Reported-by: Sebastian Parschauer 
Suggested-by: James Bottomley 
Signed-off-by: Jack Wang 
---
 drivers/scsi/scsi_lib.c | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 8106515..5a97866 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -911,9 +911,12 @@ void scsi_io_completion(struct scsi_cmnd *cmd,
unsigned int good_bytes)
  }

  /*
- * If we finished all bytes in the request we are done now.
+ * special case: failed zero length commands always need to
+ * drop down into the retry code. Otherwise, if we finished
+ * all bytes in the request we are done now.
  */
- if (!scsi_end_request(req, error, good_bytes, 0))
+ if (!(good_bytes == 0 && blk_rq_bytes(req) == 0 && result != 0) &&
+!scsi_end_request(req, error, good_bytes, 0))
  return;

  /*
-- 
1.9.1

-- 
Mit freundlichen Grüßen,
Best Regards,

Jack Wang

Linux Kernel Developer Storage
ProfitBricks GmbH  The IaaS-Company.
From 99eab170653544fa1e1bc9511ec055ba70e183d2 Mon Sep 17 00:00:00 2001
From: Jack Wang 
Date: Fri, 13 May 2016 09:53:21 +0200
Subject: [PATCH] scsi: don't fail zero length request too early

We hit IO error in our production when SYNC on multipath devices during resize
device on target side, the problem turns out scsi driver passes up as IO
error when sense data is UNIT_ATTENTION and ASC && ASCQ indicate
Capacity data has changed, even storage side sync the data properly.

Dig it further turns out we need special case on zero length commands
(currently only FLUSH), when it fails, we always need to drop down
into retry code.

Reported-by: Sebastian Parschauer 
Suggested-by: James Bottomley 
Signed-off-by: Jack Wang 
---
 drivers/scsi/scsi_lib.c | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 8106515..5a97866 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -911,9 +911,12 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
 	}
 
 	/*
-	 * If we finished all bytes in the request we are done now.
+	 * special case: failed zero length commands always need to
+	 * drop down into the retry code. Otherwise, if we finished
+	 * all bytes in the request we are done now.
 	 */
-	if (!scsi_end_request(req, error, good_bytes, 0))
+	if (!(good_bytes == 0 && blk_rq_bytes(req) == 0 && result != 0) &&
+	!scsi_end_request(req, error, good_bytes, 0))
 		return;
 
 	/*
-- 
1.9.1

Re: [PATCHv2]sd: Don't treat succeeded SYNC as error

[Jinpu Wang]

On Tue, May 10, 2016 at 6:00 PM, James Bottomley
<j...@linux.vnet.ibm.com> wrote:
> On Tue, 2016-05-10 at 17:46 +0200, Jinpu Wang wrote:
>> On Tue, May 10, 2016 at 5:08 PM, James Bottomley
>> <j...@linux.vnet.ibm.com> wrote:
> [...]
>> > Actually, I think this is symptomatic of a much bigger problem.
>> >  Now that the FS can send zero length non BLOCK_PC request, we're
>> > not treating failure correctly.  blk_update_request() will always
>> > finish them becuase they have no bytes outstanding (not having any
>> > in the first case).  So I think we need a special exception for all
>> > zero length commands which complete with a failure to allow them to
>> > retry (if required).
>>
>> Which request do you mean, I only find FLUSH?
>
> Flush is the only one currently, but zero length fs requests didn't
> exist when this bit of SCSI was coded, which is why all the code judges
> request completion on have we completed all the bytes (which is
> obviously true if you had no bytes to send in the first place).
>
>> I will test your patch.
>
> Thanks.
>
> James
>
>> Thanks,
>> Jack
>>
>> >
>> > James
>> >
>> > ---
>> >
>> > diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
>> > index 8106515..5a97866 100644
>> > --- a/drivers/scsi/scsi_lib.c
>> > +++ b/drivers/scsi/scsi_lib.c
>> > @@ -911,9 +911,12 @@ void scsi_io_completion(struct scsi_cmnd *cmd,
>> > unsigned int good_bytes)
>> > }
>> >
>> > /*
>> > -* If we finished all bytes in the request we are done now.
>> > +* special case: failed zero length commands always need to
>> > +* drop down into the retry code. Otherwise, if we finished
>> > +* all bytes in the request we are done now.
>> >  */
>> > -   if (!scsi_end_request(req, error, good_bytes, 0))
>> > +   if (!(good_bytes == 0 && blk_rq_bytes(req) == 0 && result
>> > != 0) &&
>> > +   !scsi_end_request(req, error, good_bytes, 0))
>> > return;
>> >
>> > /*
>> >
>>
>>
>>
>

Hi James,

Your patch also fix the error for me. I'm also thinking a patch like this. :)
Could you send out a formal patch, you can add my Tested-by.

Regards,
Jack
--
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: [PATCHv2]sd: Don't treat succeeded SYNC as error

[Jinpu Wang]

On Tue, May 10, 2016 at 5:08 PM, James Bottomley
<j...@linux.vnet.ibm.com> wrote:
> On Tue, 2016-05-10 at 16:48 +0200, Jinpu Wang wrote:
>> On Mon, May 9, 2016 at 6:41 PM, Jinpu Wang <
>> jinpu.w...@profitbricks.com> wrote:
>> > On Wed, May 4, 2016 at 7:02 PM, Jinpu Wang <
>> > jinpu.w...@profitbricks.com> wrote:
>> > > On Mon, May 2, 2016 at 3:44 PM, James Bottomley <
>> > > j...@linux.vnet.ibm.com> wrote:
>> > > > On Mon, 2016-05-02 at 12:05 +0200, Hannes Reinecke wrote:
>> > > > > On 04/29/2016 02:49 PM, Jinpu Wang wrote:
>> > > > > > Hi, all
>> > > > > >
>> > > > > > We hit IO error on fsync, it turns out was because sd treat
>> > > > > > succeeded
>> > > > > > SYNC as error. From what I checked in SBC spec there is no
>> > > > > > indication
>> > > > > > we should fail IO in this case, so we create this patch.
>> > > > > >
>> > > > > >
>> > > > > > Best Regards,
>> > > > > >
>> > > > > > Jack Wang
>> > > > > >
>> > > > > > v2:
>> > > > > > No change on patch itself, only resend in body as suggested
>> > > > > > by
>> > > > > > Bart,
>> > > > > > still keep the attachment in case mail client break the
>> > > > > > format.
>> > > > > >
>> > > > > > From 5d1f72d9643ce61cd9f3d312377378c43f171d0c Mon Sep 17
>> > > > > > 00:00:00
>> > > > > > 2001
>> > > > > > From: Jack Wang <jinpu.w...@profitbricks.com>
>> > > > > > Date: Mon, 25 Apr 2016 12:05:22 +0200
>> > > > > > Subject: [PATCH] sd: Don't treat succeeded SYNC as error
>> > > > > >
>> > > > > > We hit IO error in our production on multipath devices
>> > > > > > during
>> > > > > > resize
>> > > > > > device on target side, the problem turns out sd driver
>> > > > > > passes up as
>> > > > > > IO
>> > > > > > error when sense data is UNIT_ATTENTION and ASC && ASCQ
>> > > > > > indicate
>> > > > > > Capacity data has changed, even storage side sync the data
>> > > > > > properly.
>> > > > > >
>> > > > > > In order to fix this check in sd_done, report success if
>> > > > > > condition
>> > > > > > matches.
>> > > > > >
>> > > > > > Sebastian Parschauer report/analyze the bug here:
>> > > > > > https://sourceforge.net/p/scst/mailman/message/34953416/
>> > > > > >
>> > > > > > Signed-off-by: Sebastian Parschauer <s.parscha...@gmx.de>
>> > > > > > Signed-off-by: Jack Wang <jinpu.w...@profitbricks.com>
>> > > > > > ---
>> > > > > >  drivers/scsi/sd.c | 13 +
>> > > > > >  1 file changed, 13 insertions(+)
>> > > > > >
>> > > > > Well.
>> > > > > Is there anything which guarantees us that 'capacity data has
>> > > > > changed' will be the only sense code which we'll be seeing as
>> > > > > a
>> > > > > response to SYNCHRONIZE CACHE?
>> > > > > I sincerely doubt so.
>> > > > > So why don't you fall back to the default action (ie retry
>> > > > > the
>> > > > > command) whenever you hit an UNIT ATTENTION?
>> > > > > This way we would cove any resulting sense code, _and_ would
>> > > > > get rid
>> > > > > of the rather ugly special case here.
>> > > >
>> > > > Actually, why are we getting here at all?  should we be eating
>> > > > this
>> > > > unit attention once we've reported it in scsi_check_sense()?
>> > > >
>> > > > I also don't quite understand why the normal retry mechanism in
>> > > > scsi_io_completion() (called after drv->done()) isn't handling
>> > > > this.
>> > > >  We set retries on a flush command and we give sd_sync_cache
>> > > > three
>> > > > goes.  Any one of thos

Re: [PATCHv2]sd: Don't treat succeeded SYNC as error

[Jinpu Wang]

On Mon, May 9, 2016 at 6:41 PM, Jinpu Wang <jinpu.w...@profitbricks.com> wrote:
> On Wed, May 4, 2016 at 7:02 PM, Jinpu Wang <jinpu.w...@profitbricks.com> 
> wrote:
>> On Mon, May 2, 2016 at 3:44 PM, James Bottomley <j...@linux.vnet.ibm.com> 
>> wrote:
>>> On Mon, 2016-05-02 at 12:05 +0200, Hannes Reinecke wrote:
>>>> On 04/29/2016 02:49 PM, Jinpu Wang wrote:
>>>> > Hi, all
>>>> >
>>>> > We hit IO error on fsync, it turns out was because sd treat
>>>> > succeeded
>>>> > SYNC as error. From what I checked in SBC spec there is no
>>>> > indication
>>>> > we should fail IO in this case, so we create this patch.
>>>> >
>>>> >
>>>> > Best Regards,
>>>> >
>>>> > Jack Wang
>>>> >
>>>> > v2:
>>>> > No change on patch itself, only resend in body as suggested by
>>>> > Bart,
>>>> > still keep the attachment in case mail client break the format.
>>>> >
>>>> > From 5d1f72d9643ce61cd9f3d312377378c43f171d0c Mon Sep 17 00:00:00
>>>> > 2001
>>>> > From: Jack Wang <jinpu.w...@profitbricks.com>
>>>> > Date: Mon, 25 Apr 2016 12:05:22 +0200
>>>> > Subject: [PATCH] sd: Don't treat succeeded SYNC as error
>>>> >
>>>> > We hit IO error in our production on multipath devices during
>>>> > resize
>>>> > device on target side, the problem turns out sd driver passes up as
>>>> > IO
>>>> > error when sense data is UNIT_ATTENTION and ASC && ASCQ indicate
>>>> > Capacity data has changed, even storage side sync the data
>>>> > properly.
>>>> >
>>>> > In order to fix this check in sd_done, report success if condition
>>>> > matches.
>>>> >
>>>> > Sebastian Parschauer report/analyze the bug here:
>>>> > https://sourceforge.net/p/scst/mailman/message/34953416/
>>>> >
>>>> > Signed-off-by: Sebastian Parschauer <s.parscha...@gmx.de>
>>>> > Signed-off-by: Jack Wang <jinpu.w...@profitbricks.com>
>>>> > ---
>>>> >  drivers/scsi/sd.c | 13 +
>>>> >  1 file changed, 13 insertions(+)
>>>> >
>>>> Well.
>>>> Is there anything which guarantees us that 'capacity data has
>>>> changed' will be the only sense code which we'll be seeing as a
>>>> response to SYNCHRONIZE CACHE?
>>>> I sincerely doubt so.
>>>> So why don't you fall back to the default action (ie retry the
>>>> command) whenever you hit an UNIT ATTENTION?
>>>> This way we would cove any resulting sense code, _and_ would get rid
>>>> of the rather ugly special case here.
>>>
>>> Actually, why are we getting here at all?  should we be eating this
>>> unit attention once we've reported it in scsi_check_sense()?
>>>
>>> I also don't quite understand why the normal retry mechanism in
>>> scsi_io_completion() (called after drv->done()) isn't handling this.
>>>  We set retries on a flush command and we give sd_sync_cache three
>>> goes.  Any one of those should also cause the CC/UA to be ignored.
>>>
>>> James
>>>
>>>
>>
>> Sorry for delay, I agree safer to retry this command.
>> I checked the code path, in scsi_io_completion, we call
>> __scsi_error_from_host_byte for FLUSH request,
>> and we set error to EIO by default, somehow the code report error
>> directly to user space without retry.
>> [  647.206270] sd 1:0:0:0: [sdb] tag#0 Send: scmd 0x8800b6558480
>> [  647.206422] sd 1:0:0:0: [sdb] tag#0 CDB: Synchronize Cache(10) 35
>> 00 00 00 00 00 00 00 00 00
>> [  647.209748] sd 1:0:0:0: Capacity data has changed
>> [  647.209896] sd 1:0:0:0: [sdb] tag#0 Done: SUCCESS Result:
>> hostbyte=DID_OK driverbyte=DRIVER_OK
>> [  647.210157] sd 1:0:0:0: [sdb] tag#0 CDB: Synchronize Cache(10) 35
>> 00 00 00 00 00 00 00 00 00
>> [  647.210419] sd 1:0:0:0: [sdb] tag#0 Sense Key : Unit Attention [current]
>> [  647.210567] sd 1:0:0:0: [sdb] tag#0 Add. Sense: Capacity data has changed
>> [  647.210741] sd 1:0:0:0: [sdb] tag#0 scsi host busy 1 failed 0
>> [  647.210888] sd 1:0:0:0: Notifying upper driver of completion (result 
>> 802)
>> [  647.211035] sd 1:0:0:0: [sdb] tag#0 sd_done: completed 0 of 0 bytes
>> [  647.211

Re: [PATCHv2]sd: Don't treat succeeded SYNC as error

[Jinpu Wang]

On Wed, May 4, 2016 at 7:02 PM, Jinpu Wang <jinpu.w...@profitbricks.com> wrote:
> On Mon, May 2, 2016 at 3:44 PM, James Bottomley <j...@linux.vnet.ibm.com> 
> wrote:
>> On Mon, 2016-05-02 at 12:05 +0200, Hannes Reinecke wrote:
>>> On 04/29/2016 02:49 PM, Jinpu Wang wrote:
>>> > Hi, all
>>> >
>>> > We hit IO error on fsync, it turns out was because sd treat
>>> > succeeded
>>> > SYNC as error. From what I checked in SBC spec there is no
>>> > indication
>>> > we should fail IO in this case, so we create this patch.
>>> >
>>> >
>>> > Best Regards,
>>> >
>>> > Jack Wang
>>> >
>>> > v2:
>>> > No change on patch itself, only resend in body as suggested by
>>> > Bart,
>>> > still keep the attachment in case mail client break the format.
>>> >
>>> > From 5d1f72d9643ce61cd9f3d312377378c43f171d0c Mon Sep 17 00:00:00
>>> > 2001
>>> > From: Jack Wang <jinpu.w...@profitbricks.com>
>>> > Date: Mon, 25 Apr 2016 12:05:22 +0200
>>> > Subject: [PATCH] sd: Don't treat succeeded SYNC as error
>>> >
>>> > We hit IO error in our production on multipath devices during
>>> > resize
>>> > device on target side, the problem turns out sd driver passes up as
>>> > IO
>>> > error when sense data is UNIT_ATTENTION and ASC && ASCQ indicate
>>> > Capacity data has changed, even storage side sync the data
>>> > properly.
>>> >
>>> > In order to fix this check in sd_done, report success if condition
>>> > matches.
>>> >
>>> > Sebastian Parschauer report/analyze the bug here:
>>> > https://sourceforge.net/p/scst/mailman/message/34953416/
>>> >
>>> > Signed-off-by: Sebastian Parschauer <s.parscha...@gmx.de>
>>> > Signed-off-by: Jack Wang <jinpu.w...@profitbricks.com>
>>> > ---
>>> >  drivers/scsi/sd.c | 13 +
>>> >  1 file changed, 13 insertions(+)
>>> >
>>> Well.
>>> Is there anything which guarantees us that 'capacity data has
>>> changed' will be the only sense code which we'll be seeing as a
>>> response to SYNCHRONIZE CACHE?
>>> I sincerely doubt so.
>>> So why don't you fall back to the default action (ie retry the
>>> command) whenever you hit an UNIT ATTENTION?
>>> This way we would cove any resulting sense code, _and_ would get rid
>>> of the rather ugly special case here.
>>
>> Actually, why are we getting here at all?  should we be eating this
>> unit attention once we've reported it in scsi_check_sense()?
>>
>> I also don't quite understand why the normal retry mechanism in
>> scsi_io_completion() (called after drv->done()) isn't handling this.
>>  We set retries on a flush command and we give sd_sync_cache three
>> goes.  Any one of those should also cause the CC/UA to be ignored.
>>
>> James
>>
>>
>
> Sorry for delay, I agree safer to retry this command.
> I checked the code path, in scsi_io_completion, we call
> __scsi_error_from_host_byte for FLUSH request,
> and we set error to EIO by default, somehow the code report error
> directly to user space without retry.
> [  647.206270] sd 1:0:0:0: [sdb] tag#0 Send: scmd 0x8800b6558480
> [  647.206422] sd 1:0:0:0: [sdb] tag#0 CDB: Synchronize Cache(10) 35
> 00 00 00 00 00 00 00 00 00
> [  647.209748] sd 1:0:0:0: Capacity data has changed
> [  647.209896] sd 1:0:0:0: [sdb] tag#0 Done: SUCCESS Result:
> hostbyte=DID_OK driverbyte=DRIVER_OK
> [  647.210157] sd 1:0:0:0: [sdb] tag#0 CDB: Synchronize Cache(10) 35
> 00 00 00 00 00 00 00 00 00
> [  647.210419] sd 1:0:0:0: [sdb] tag#0 Sense Key : Unit Attention [current]
> [  647.210567] sd 1:0:0:0: [sdb] tag#0 Add. Sense: Capacity data has changed
> [  647.210741] sd 1:0:0:0: [sdb] tag#0 scsi host busy 1 failed 0
> [  647.210888] sd 1:0:0:0: Notifying upper driver of completion (result 
> 802)
> [  647.211035] sd 1:0:0:0: [sdb] tag#0 sd_done: completed 0 of 0 bytes
> [  647.211182] sd 1:0:0:0: [sdb] tag#0 0 sectors total, 0 bytes done, error -5
> [  647.211330] blk_update_request: I/O error, dev sdb, sector 0
>
> Will figure out why retry doesn't work.
>
> Thanks James and Hannes for all your input.
>
> Regards,
> Jack

Hi James, Hannes and all,

I find out it's code below which report error directly back to user
space without any retry.
 913 /*
 914  * If we finished all bytes in the request we are done now.
 915  */
 916 if (!scsi_end_request(req, error, good_bytes, 0))
 917 return;

But not sure, what's the best way to fix the behavior to let it retry,
 maybe add condition with sense key && asc && ascq direct go to
requeue before line 913?

Thanks
Jack
--
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

[PATCHv2]sd: Don't treat succeeded SYNC as error

[Jinpu Wang]

Hi, all

We hit IO error on fsync, it turns out was because sd treat succeeded
SYNC as error. From what I checked in SBC spec there is no indication
we should fail IO in this case, so we create this patch.


Best Regards,

Jack Wang

v2:
No change on patch itself, only resend in body as suggested by Bart,
still keep the attachment in case mail client break the format.

>From 5d1f72d9643ce61cd9f3d312377378c43f171d0c Mon Sep 17 00:00:00 2001
From: Jack Wang 
Date: Mon, 25 Apr 2016 12:05:22 +0200
Subject: [PATCH] sd: Don't treat succeeded SYNC as error

We hit IO error in our production on multipath devices during resize
device on target side, the problem turns out sd driver passes up as IO
error when sense data is UNIT_ATTENTION and ASC && ASCQ indicate
Capacity data has changed, even storage side sync the data properly.

In order to fix this check in sd_done, report success if condition
matches.

Sebastian Parschauer report/analyze the bug here:
https://sourceforge.net/p/scst/mailman/message/34953416/

Signed-off-by: Sebastian Parschauer 
Signed-off-by: Jack Wang 
---
 drivers/scsi/sd.c | 13 +
 1 file changed, 13 insertions(+)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 5a5457a..e9bfe01 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -1833,6 +1833,19 @@ static int sd_done(struct scsi_cmnd *SCpnt)
  }
  }
  break;
+ case UNIT_ATTENTION:
+ /* Capacity data has changed */
+ if (sshdr.asc == 0x2a && sshdr.ascq == 0x09) {
+ switch (op) {
+ /* don't treat succeeded fsync() as error */
+ case SYNCHRONIZE_CACHE:
+ case SYNCHRONIZE_CACHE_16:
+ if (good_bytes == scsi_bufflen(SCpnt))
+ SCpnt->result = 0;
+ break;
+ }
+ }
+ break;
  default:
  break;
  }
-- 
1.9.1
From 5d1f72d9643ce61cd9f3d312377378c43f171d0c Mon Sep 17 00:00:00 2001
From: Jack Wang 
Date: Mon, 25 Apr 2016 12:05:22 +0200
Subject: [PATCH] sd: Don't treat succeeded SYNC as error

We hit IO error in our production on multipath devices during resize
device on target side, the problem turns out sd driver passes up as IO
error when sense data is UNIT_ATTENTION and ASC && ASCQ indicate
Capacity data has changed, even storage side sync the data properly.

In order to fix this check in sd_done, report success if condition
matches.

Sebastian Parschauer report/analyze the bug here:
https://sourceforge.net/p/scst/mailman/message/34953416/

Signed-off-by: Sebastian Parschauer 
Signed-off-by: Jack Wang 
---
 drivers/scsi/sd.c | 13 +
 1 file changed, 13 insertions(+)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 5a5457a..e9bfe01 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -1833,6 +1833,19 @@ static int sd_done(struct scsi_cmnd *SCpnt)
 			}
 		}
 		break;
+	case UNIT_ATTENTION:
+		/* Capacity data has changed */
+		if (sshdr.asc == 0x2a && sshdr.ascq == 0x09) {
+			switch (op) {
+			/* don't treat succeeded fsync() as error */
+			case SYNCHRONIZE_CACHE:
+			case SYNCHRONIZE_CACHE_16:
+if (good_bytes == scsi_bufflen(SCpnt))
+	SCpnt->result = 0;
+	break;
+			}
+		}
+		break;
 	default:
 		break;
 	}
-- 
1.9.1

Re: [PATCH]sd: Don't treat succeeded SYNC as error

[Jinpu Wang]

On Mon, Apr 25, 2016 at 10:28 PM, Bart Van Assche
<bart.vanass...@sandisk.com> wrote:
> On 04/25/2016 03:36 AM, Jinpu Wang wrote:
>>
>> We hit IO error on fsync, it turns out was because sd treat succeeded
>> SYNC as error. From what I checked in SBC spec there is no indication
>> we should fail IO in this case, so we create this patch.
>
>
> Please follow the rules in Documentation/SubmittingPatches and submit
> patches inline. Regarding the patch itself: why is the switch(op) needed?
> Can it be left out? And regarding (sshdr.asc == 0x2a && sshdr.ascq == 0x09):
> which other unit attentions should cause SCSI commands to succeed instead of
> to fail?
>
> Bart.

Hi Bart,

Thanks for looking into this.
Sure, I can resubmit inline.
Because our test only cover this, unconditional treat other commands
without test may break other application.

At least for READ/WRITE operation, ML will requeue it because not all
bytes transfered:
[ 1794.236001] sd 1:0:0:0: Capacity data has changed
[ 1794.236141] sd 1:0:0:0: [sdb] tag#0 Done: SUCCESS Result:
hostbyte=DID_OK driverbyte=DRIVER_OK
[ 1794.236381] sd 1:0:0:0: [sdb] tag#0 CDB: Read(10) 28 00 00 00 00 00
00 00 01 00
[ 1794.236609] sd 1:0:0:0: [sdb] tag#0 Sense Key : Unit Attention [current]
[ 1794.236725] sd 1:0:0:0: [sdb] tag#0 Add. Sense: Capacity data has changed
[ 1794.236839] sd 1:0:0:0: [sdb] tag#0 scsi host busy 1 failed 0
[ 1794.236981] sd 1:0:0:0: Notifying upper driver of completion (result 802)
[ 1794.237096] sd 1:0:0:0: [sdb] tag#0 sd_done: completed 0 of 512 bytes
[ 1794.237210] sd 1:0:0:0: [sdb] tag#0 1 sectors total, 0 bytes done.
[ 1794.237325] sd 1:0:0:0: [sdb] tag#0 Inserting command
88022b9d4780 into mlqueue
[ 1794.238631] sd 1:0:0:0: [sdb] sd_open
[ 1794.238745] sd 1:0:0:0: scsi_block_when_processing_errors: rtn: 1
[ 1794.238864] sd 1:0:0:0: scsi_block_when_processing_errors: rtn: 1
[ 1794.238989] sd 1:0:0:0: scsi_block_when_processing_errors: rtn: 1
[ 1794.239107] sd 1:0:0:0: unblocking device at zero depth
[ 1794.239224] sd 1:0:0:0: [sdb] tag#0 Send: scmd 0x88022b9d4780
[ 1794.239338] sd 1:0:0:0: [sdb] tag#0 CDB: Read(10) 28 00 00 00 00 00
00 00 01 00
[ 1794.239592] sd 1:0:0:0: [sdb] tag#1 Send: scmd 0x8802296af380
[ 1794.239675] sd 1:0:0:0: [sdb] tag#0 Done: SUCCESS Result:
hostbyte=DID_OK driverbyte=DRIVER_OK
[ 1794.239677] sd 1:0:0:0: [sdb] tag#0 CDB: Read(10) 28 00 00 00 00 00
00 00 01 00
[ 1794.239678] sd 1:0:0:0: [sdb] tag#0 scsi host busy 2 failed 0
[ 1794.239679] sd 1:0:0:0: Notifying upper driver of completion (result 0)

But for SYNC CACHE:

[   84.450906] sd 6:0:0:1: [sdc] sd_open
[   84.451008] sd 6:0:0:1: scsi_block_when_processing_errors: rtn: 1
[   84.451084] sd 6:0:0:1: [sdc] tag#1 Send: scmd 0x880210357080
[   84.451143] sd 6:0:0:1: [sdc] tag#1 CDB: Synchronize Cache(10) 35
00 00 00 00 00 00 00 00 00
[   84.456081] sd 6:0:0:1: [sdc] tag#0 Done: SUCCESS Result:
hostbyte=DID_OK driverbyte=DRIVER_OK
[   84.456209] sd 6:0:0:1: [sdc] tag#0 CDB: Read(10) 28 00 00 00 01 10
00 00 08 00
[   84.456295] sd 6:0:0:1: [sdc] tag#0 scsi host busy 2 failed 0
[   84.456374] sd 6:0:0:1: Notifying upper driver of completion (result 0)
[   84.456438] sd 6:0:0:1: [sdc] tag#0 sd_done: completed 4096 of 4096 bytes
[   84.456498] sd 6:0:0:1: [sdc] tag#0 8 sectors total, 4096 bytes done.
[   84.456595] sd 6:0:0:1: Capacity data has changed
[   84.456612] sd 6:0:0:1: [sdc] sd_setup_read_write_cmnd: block=280, count=8
[   84.456616] sd 6:0:0:1: [sdc] block=280
[   84.456620] sd 6:0:0:1: [sdc] reading 8/8 512 byte blocks.
[   84.456628] sd 6:0:0:1: [sdc] tag#0 Send: scmd 0x8800ca41ed80
[   84.456634] sd 6:0:0:1: [sdc] tag#0 CDB: Read(10) 28 00 00 00 01 18
00 00 08 00
[   84.457003] sd 6:0:0:1: [sdc] tag#1 Done: SUCCESS Result:
hostbyte=DID_OK driverbyte=DRIVER_OK
[   84.457090] sd 6:0:0:1: [sdc] tag#1 CDB: Synchronize Cache(10) 35
00 00 00 00 00 00 00 00 00
[   84.457211] sd 6:0:0:1: [sdc] tag#1 Sense Key : Unit Attention [current]
[   84.457272] sd 6:0:0:1: [sdc] tag#1 Add. Sense: Capacity data has changed
[   84.457331] sd 6:0:0:1: [sdc] tag#1 scsi host busy 2 failed 0
[   84.457387] sd 6:0:0:1: Notifying upper driver of completion (result 802)
[   84.457445] sd 6:0:0:1: [sdc] tag#1 sd_done: completed 0 of 0 bytes
[   84.457503] sd 6:0:0:1: [sdc] tag#1 0 sectors total, 0 bytes done.
[   84.457562] blk_update_request: I/O error, dev sdc, sector 0

It completed with error directly.
We don't have failure case with our UA yet.

Thanks,
Jack
--
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]sd: Don't treat succeeded SYNC as error

[Jinpu Wang]

Hi, all

We hit IO error on fsync, it turns out was because sd treat succeeded
SYNC as error. From what I checked in SBC spec there is no indication
we should fail IO in this case, so we create this patch.


Best Regards,

Jack Wang
From 5d1f72d9643ce61cd9f3d312377378c43f171d0c Mon Sep 17 00:00:00 2001
From: Jack Wang 
Date: Mon, 25 Apr 2016 12:05:22 +0200
Subject: [PATCH] sd: Don't treat succeeded SYNC as error

We hit IO error in our production on multipath devices during resize
device on target side, the problem turns out sd driver passes up as IO
error when sense data is UNIT_ATTENTION and ASC && ASCQ indicate
Capacity data has changed, even storage side sync the data properly.

In order to fix this check in sd_done, report success if condition
matches.

Sebastian Parschauer report/analyze the bug here:
https://sourceforge.net/p/scst/mailman/message/34953416/

Signed-off-by: Sebastian Parschauer 
Signed-off-by: Jack Wang 
---
 drivers/scsi/sd.c | 13 +
 1 file changed, 13 insertions(+)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 5a5457a..e9bfe01 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -1833,6 +1833,19 @@ static int sd_done(struct scsi_cmnd *SCpnt)
 			}
 		}
 		break;
+	case UNIT_ATTENTION:
+		/* Capacity data has changed */
+		if (sshdr.asc == 0x2a && sshdr.ascq == 0x09) {
+			switch (op) {
+			/* don't treat succeeded fsync() as error */
+			case SYNCHRONIZE_CACHE:
+			case SYNCHRONIZE_CACHE_16:
+if (good_bytes == scsi_bufflen(SCpnt))
+	SCpnt->result = 0;
+	break;
+			}
+		}
+		break;
 	default:
 		break;
 	}
-- 
1.9.1

Re: [PATCH] pm80xx: Remove bogus address masking in pm8001_ioremap()

[Jinpu Wang]

On Wed, Apr 13, 2016 at 11:26 PM, David Daney  wrote:
> From: David Daney 
>
> It is unclear what the original intent of the masking was, but it is
> clearly incorrect to truncate a physical address before calling
> ioremap().  On systems where there are valid physical address bits
> above bit-31 (arm64 for example) the result is an eventual OOPs when
> initializing the driver.
>
> Remove the bogus code to fix it.
>
> Signed-off-by: David Daney 
> ---
>  drivers/scsi/pm8001/pm8001_init.c | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/drivers/scsi/pm8001/pm8001_init.c 
> b/drivers/scsi/pm8001/pm8001_init.c
> index 062ab34..6bd7bf4 100644
> --- a/drivers/scsi/pm8001/pm8001_init.c
> +++ b/drivers/scsi/pm8001/pm8001_init.c
> @@ -418,8 +418,6 @@ static int pm8001_ioremap(struct pm8001_hba_info 
> *pm8001_ha)
> if (pci_resource_flags(pdev, bar) & IORESOURCE_MEM) {
> pm8001_ha->io_mem[logicalBar].membase =
> pci_resource_start(pdev, bar);
> -   pm8001_ha->io_mem[logicalBar].membase &=
> -   (u32)PCI_BASE_ADDRESS_MEM_MASK;
> pm8001_ha->io_mem[logicalBar].memsize =
> pci_resource_len(pdev, bar);
> pm8001_ha->io_mem[logicalBar].memvirtaddr =
> --
> 1.8.3.1
>

Thanks, looks good to me.
Acked-by: Jack Wang 
--
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] mvsas: don't allow negative timeouts

[Jinpu Wang]

On Fri, Nov 13, 2015 at 3:23 PM, Dan Carpenter  wrote:
> There is a static checker warning here because "val" is controlled by
> the user and we have a upper bound on it but allow negative numbers.
> "val" appears to be a timeout in usec so this bug probably means we
> have a longer timeout than we should.  Let's fix this by changing "val"
> to unsigned.
>
> Signed-off-by: Dan Carpenter 
> ---
> Checkpatch has several complaints about this code but I left it as-is.
>
> diff --git a/drivers/scsi/mvsas/mv_init.c b/drivers/scsi/mvsas/mv_init.c
> index 90fdf0e..675e7fa 100644
> --- a/drivers/scsi/mvsas/mv_init.c
> +++ b/drivers/scsi/mvsas/mv_init.c
> @@ -758,7 +758,7 @@ mvs_store_interrupt_coalescing(struct device *cdev,
> struct device_attribute *attr,
> const char *buffer, size_t size)
>  {
> -   int val = 0;
> +   unsigned int val = 0;
> struct mvs_info *mvi = NULL;
> struct Scsi_Host *shost = class_to_shost(cdev);
> struct sas_ha_struct *sha = SHOST_TO_SAS_HA(shost);
> @@ -766,7 +766,7 @@ mvs_store_interrupt_coalescing(struct device *cdev,
> if (buffer == NULL)
> return size;
>
> -   if (sscanf(buffer, "%d", ) != 1)
> +   if (sscanf(buffer, "%u", ) != 1)
> return -EINVAL;
>
> if (val >= 0x1) {

Looks good to me.
Reviewed-by: Jack Wang 

-- 
Mit freundlichen Grüßen,
Best Regards,

Jack Wang

Linux Kernel Developer Storage
ProfitBricks GmbH  The IaaS-Company.
--
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 1/3] isci: isci: remove SCSI host before detaching from SAS transport

[Jinpu Wang]

Hi Christoph,

On Thu, Nov 5, 2015 at 11:23 AM, Christoph Hellwig  wrote:
> Hi Jack,
>
> all three patches look fine to me, but it seems like your mailer mangled
> the whitespaces.

Thanks, I've resend with git send-email.
Sorry for inconvenient.

-- 
Mit freundlichen Grüßen,
Best Regards,

Jack Wang

Linux Kernel Developer Storage
ProfitBricks GmbH  The IaaS-Company.

ProfitBricks GmbH
Greifswalder Str. 207
D - 10405 Berlin
Tel: +49 30 5770083-42
Fax: +49 30 5770085-98
Email: jinpu.w...@profitbricks.com
URL: http://www.profitbricks.de

Sitz der Gesellschaft: Berlin.
Registergericht: Amtsgericht Charlottenburg, HRB 125506 B.
Geschäftsführer: Andreas Gauger, Achim Weiss.
--
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 1/3] isci: isci: remove SCSI host before detaching from SAS transport

[Jinpu Wang]

>From b1fb56b6d98610251253dc4b858df509df08e813 Mon Sep 17 00:00:00 2001
From: Jack Wang 
Date: Wed, 4 Nov 2015 09:46:05 +0100
Subject: [PATCH 1/3] isci: remove SCSI host before detaching from SAS
 transport

commit cff549e4860f ("scsi: proper state checking and module refcount
handling in scsi_device_get")
, the reference count of scsi device was changed, which could lead to when
rmmod with at least on drive attached, SCSI error handle will
run into infinite loop, and lockup the system.

Fix it by remove scsi host first, this way scsi core will not send
commands down after detaching SAS transport.

This is a follow up fix for Benjamin's fix for pm80xx.

See also:
http://www.spinics.net/lists/linux-scsi/msg90088.html

Signed-off-by: Jack Wang 
---
 drivers/scsi/isci/init.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/isci/init.c b/drivers/scsi/isci/init.c
index 0dfcabe..9e0d069 100644
--- a/drivers/scsi/isci/init.c
+++ b/drivers/scsi/isci/init.c
@@ -272,11 +272,11 @@ static void isci_unregister(struct isci_host *isci_host)
  if (!isci_host)
  return;

+ shost = to_shost(isci_host);
+ scsi_remove_host(shost);
  sas_unregister_ha(_host->sas_ha);

- shost = to_shost(isci_host);
  sas_remove_host(shost);
- scsi_remove_host(shost);
  scsi_host_put(shost);
 }

-- 
1.9.1


-- 
Mit freundlichen Grüßen,
Best Regards,

Jack Wang

Linux Kernel Developer Storage
ProfitBricks GmbH  The IaaS-Company.

ProfitBricks GmbH
Greifswalder Str. 207
D - 10405 Berlin
Tel: +49 30 5770083-42
Fax: +49 30 5770085-98
Email: jinpu.w...@profitbricks.com
URL: http://www.profitbricks.de

Sitz der Gesellschaft: Berlin.
Registergericht: Amtsgericht Charlottenburg, HRB 125506 B.
Geschäftsführer: Andreas Gauger, Achim Weiss.
--
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 2/3] aic94xx: remove SCSI host before detaching from SAS transport

[Jinpu Wang]

>From 3aae177e7aa89fa84ae5c60ed5f10056610d Mon Sep 17 00:00:00 2001
From: Jack Wang 
Date: Wed, 4 Nov 2015 10:01:41 +0100
Subject: [PATCH 2/3] aic94xx: remove SCSI host before detaching from SAS
 transport

commit cff549e4860f ("scsi: proper state checking and module refcount
handling in scsi_device_get")
the reference count of scsi device was changed, which could lead to
when rmmod with at least on drive attached, SCSI error handle will
run into infinite loop, and lockup the system.

Fix it by remove scsi host first, this way scsi core will not send
commands down after detaching SAS transport.

This is a follow up fix for Benjamin's fix for pm80xx.

See also:
http://www.spinics.net/lists/linux-scsi/msg90088.html

Signed-off-by: Jack Wang 
---
 drivers/scsi/aic94xx/aic94xx_init.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/aic94xx/aic94xx_init.c
b/drivers/scsi/aic94xx/aic94xx_init.c
index f6c336b..4b56976 100644
--- a/drivers/scsi/aic94xx/aic94xx_init.c
+++ b/drivers/scsi/aic94xx/aic94xx_init.c
@@ -704,10 +704,10 @@ static int asd_unregister_sas_ha(struct
asd_ha_struct *asd_ha)
 {
  int err;

+ scsi_remove_host(asd_ha->sas_ha.core.shost);
  err = sas_unregister_ha(_ha->sas_ha);

  sas_remove_host(asd_ha->sas_ha.core.shost);
- scsi_remove_host(asd_ha->sas_ha.core.shost);
  scsi_host_put(asd_ha->sas_ha.core.shost);

  kfree(asd_ha->sas_ha.sas_phy);
-- 
1.9.1


-- 
Mit freundlichen Grüßen,
Best Regards,

Jack Wang

Linux Kernel Developer Storage
ProfitBricks GmbH  The IaaS-Company.

ProfitBricks GmbH
Greifswalder Str. 207
D - 10405 Berlin
Tel: +49 30 5770083-42
Fax: +49 30 5770085-98
Email: jinpu.w...@profitbricks.com
URL: http://www.profitbricks.de

Sitz der Gesellschaft: Berlin.
Registergericht: Amtsgericht Charlottenburg, HRB 125506 B.
Geschäftsführer: Andreas Gauger, Achim Weiss.
--
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 3/3] mvsas: remove SCSI host before detaching from SAS transport

[Jinpu Wang]

>From 6c3cb7d4df1e164008338c922cd99149a6a8191b Mon Sep 17 00:00:00 2001
From: Jack Wang 
Date: Wed, 4 Nov 2015 10:05:01 +0100
Subject: [PATCH 3/3] mvsas: remove SCSI host before detaching from SAS
 transport

commit cff549e4860f ("scsi: proper state checking and module refcount
handling in scsi_device_get")

the reference count of scsi device was changed, which could lead to
when rmmod with at least on drive attached, SCSI error handle will
run into infinite loop, and lockup the system.

Fix it by remove scsi host first, this way scsi core will not send
commands down after detaching SAS transport.

This is a follow up fix for Benjamin's fix for pm80xx.

See also:
http://www.spinics.net/lists/linux-scsi/msg90088.html

Signed-off-by: Jack Wang 
---
 drivers/scsi/mvsas/mv_init.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/mvsas/mv_init.c b/drivers/scsi/mvsas/mv_init.c
index e2d555c..1960d95 100644
--- a/drivers/scsi/mvsas/mv_init.c
+++ b/drivers/scsi/mvsas/mv_init.c
@@ -641,9 +641,9 @@ static void mvs_pci_remove(struct pci_dev *pdev)
  tasklet_kill(&((struct mvs_prv_info *)sha->lldd_ha)->mv_tasklet);
 #endif

+ scsi_remove_host(mvi->shost);
  sas_unregister_ha(sha);
  sas_remove_host(mvi->shost);
- scsi_remove_host(mvi->shost);

  MVS_CHIP_DISP->interrupt_disable(mvi);
  free_irq(mvi->pdev->irq, sha);
-- 
1.9.1


-- 
Mit freundlichen Grüßen,
Best Regards,

Jack Wang

Linux Kernel Developer Storage
ProfitBricks GmbH  The IaaS-Company.

ProfitBricks GmbH
Greifswalder Str. 207
D - 10405 Berlin
Tel: +49 30 5770083-42
Fax: +49 30 5770085-98
Email: jinpu.w...@profitbricks.com
URL: http://www.profitbricks.de

Sitz der Gesellschaft: Berlin.
Registergericht: Amtsgericht Charlottenburg, HRB 125506 B.
Geschäftsführer: Andreas Gauger, Achim Weiss.
--
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 V2 2/8] pm80xx: Corrected device state changes in I_T_Nexus_Reset.

[Jinpu Wang]

On Fri, Aug 14, 2015 at 9:15 AM, Viswas G viswa...@pmcs.com wrote:
 On Thu, Aug 13, 2015 at 6:13 PM, Jinpu Wang jinpu.w...@profitbricks.com 
 wrote:
 Hi

 On Tue, Aug 11, 2015 at 11:36 AM,  viswa...@pmcs.com wrote:
 From: Viswas G viswa...@pmcs.com

 In Nexus reset the device state request are not needed.

 Changes from V1:
 Device state change request has been removed as the firmware
 will handle it during internal cleanup. Also updated the
 proper return value in case of failures.

 If above is true, should we remove the device state change command
 support at all?


 We can't remove the state change command as it is required for
 some task management command such as ABORT_TASK.


Ok, thanks

-- 
Mit freundlichen Grüßen,
Best Regards,

Jack Wang

Linux Kernel Developer Storage
ProfitBricks GmbH  The IaaS-Company.

ProfitBricks GmbH
Greifswalder Str. 207
D - 10405 Berlin
Tel: +49 30 5770083-42
Fax: +49 30 5770085-98
Email: jinpu.w...@profitbricks.com
URL: http://www.profitbricks.de

Sitz der Gesellschaft: Berlin.
Registergericht: Amtsgericht Charlottenburg, HRB 125506 B.
Geschäftsführer: Andreas Gauger, Achim Weiss.
--
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 V2 2/8] pm80xx: Corrected device state changes in I_T_Nexus_Reset.

[Jinpu Wang]

Hi

On Tue, Aug 11, 2015 at 11:36 AM,  viswa...@pmcs.com wrote:
 From: Viswas G viswa...@pmcs.com

 In Nexus reset the device state request are not needed.

 Changes from V1:
 Device state change request has been removed as the firmware
 will handle it during internal cleanup. Also updated the
 proper return value in case of failures.

If above is true, should we remove the device state change command
support at all?

Other than that, please feel free to add:
Acked-by: Jack Wang jinpu.w...@profitbricks.com

Thanks
Jack



 Signed-off-by: Viswas G viswa...@pmcs.com

 Reviewed-by: Suresh Thiagarajan suresh.thiagara...@pmcs.com
 ---
  drivers/scsi/pm8001/pm8001_sas.c |   18 +-
  drivers/scsi/pm8001/pm8001_sas.h |8 
  2 files changed, 21 insertions(+), 5 deletions(-)

 diff --git a/drivers/scsi/pm8001/pm8001_sas.c 
 b/drivers/scsi/pm8001/pm8001_sas.c
 index b93f289..48f4627 100644
 --- a/drivers/scsi/pm8001/pm8001_sas.c
 +++ b/drivers/scsi/pm8001/pm8001_sas.c
 @@ -975,19 +975,27 @@ int pm8001_I_T_nexus_reset(struct domain_device *dev)
 phy = sas_get_local_phy(dev);

 if (dev_is_sata(dev)) {
 -   DECLARE_COMPLETION_ONSTACK(completion_setstate);
 if (scsi_is_sas_phy_local(phy)) {
 rc = 0;
 goto out;
 }
 rc = sas_phy_reset(phy, 1);
 +   if (rc) {
 +   PM8001_EH_DBG(pm8001_ha,
 +   pm8001_printk(phy reset failed for device %x\n
 +   with rc %d\n, pm8001_dev-device_id, rc));
 +   rc = TMF_RESP_FUNC_FAILED;
 +   goto out;
 +   }
 msleep(2000);
 rc = pm8001_exec_internal_task_abort(pm8001_ha, pm8001_dev ,
 dev, 1, 0);
 -   pm8001_dev-setds_completion = completion_setstate;
 -   rc = PM8001_CHIP_DISP-set_dev_state_req(pm8001_ha,
 -   pm8001_dev, 0x01);
 -   wait_for_completion(completion_setstate);
 +   if (rc) {
 +   PM8001_EH_DBG(pm8001_ha,
 +   pm8001_printk(task abort failed %x\n
 +   with rc %d\n, pm8001_dev-device_id, rc));
 +   rc = TMF_RESP_FUNC_FAILED;
 +   }
 } else {
 rc = sas_phy_reset(phy, 1);
 msleep(2000);
 diff --git a/drivers/scsi/pm8001/pm8001_sas.h 
 b/drivers/scsi/pm8001/pm8001_sas.h
 index 8dd8b78..c9736cc 100644
 --- a/drivers/scsi/pm8001/pm8001_sas.h
 +++ b/drivers/scsi/pm8001/pm8001_sas.h
 @@ -569,6 +569,14 @@ struct pm8001_fw_image_header {
  #defineNCQ_READ_LOG_FLAG   0x8000
  #defineNCQ_ABORT_ALL_FLAG  0x4000
  #defineNCQ_2ND_RLE_FLAG0x2000
 +
 +/* Device states */
 +#define DS_OPERATIONAL 0x01
 +#define DS_PORT_IN_RESET   0x02
 +#define DS_IN_RECOVERY 0x03
 +#define DS_IN_ERROR0x04
 +#define DS_NON_OPERATIONAL 0x07
 +
  /**
   * brief param structure for firmware flash update.
   */
 --
 1.7.1




-- 
Mit freundlichen Grüßen,
Best Regards,

Jack Wang

Linux Kernel Developer Storage
ProfitBricks GmbH  The IaaS-Company.

ProfitBricks GmbH
Greifswalder Str. 207
D - 10405 Berlin
Tel: +49 30 5770083-42
Fax: +49 30 5770085-98
Email: jinpu.w...@profitbricks.com
URL: http://www.profitbricks.de

Sitz der Gesellschaft: Berlin.
Registergericht: Amtsgericht Charlottenburg, HRB 125506 B.
Geschäftsführer: Andreas Gauger, Achim Weiss.
--
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