[PATCH] target: Use WARNON_NON_RT(!irqs_disabled())

[PATCH] target: Use WARNON_NON_RT(!irqs_disabled())

[Arnaldo Carvalho de Melo]

Hi,

	We got a report where this WARN_ON got triggered:

[ 7552.799997] ------------[ cut here ]------------
[ 7552.800016] WARNING: CPU: 7 PID: 1090 at drivers/target/target_core_transport.c:3009 __transport_check_aborted_status+0x153/0x190 [target_core_mod]
[ 7552.800037] Modules linked in: target_core_user uio target_core_pscsi target_core_file target_core_iblock ib_srpt ib_srp scsi_transport_srp scsi_tgt xt_CHECKSUM iptable_mangle ipt_MASQUERADE nf_nat_masquerade_ipv4 iptable_nat nf_nat_ipv4 nf_nat nf_conntrack_ipv4 nf_defrag_ipv4 xt_conntrack nf_conntrack ipt_REJECT nf_reject_ipv4 tun ebtable_filter ebtables ip6table_filter ip6_tables iptable_filter bridge stp llc ib_isert iscsi_target_mod target_core_mod ib_ucm rpcrdma mlx5_ib sunrpc rdma_ucm ib_uverbs ib_iser rdma_cm iw_cm libiscsi ib_umad ib_ipoib scsi_transport_iscsi ib_cm sb_edac intel_powerclamp coretemp intel_rapl iosf_mbi crc32_pclmul ghash_clmulni_intel aesni_intel lrw gf128mul glue_helper ablk_helper cryptd iTCO_wdt iTCO_vendor_support hfi1 ipmi_ssif sg rdmavt ib_core hpilo hpwdt pcspkr ipmi_si
[ 7552.800055]  ipmi_devintf ipmi_msghandler wmi acpi_power_meter ioatdma dca shpchp pcc_cpufreq lpc_ich ip_tables xfs libcrc32c mgag200 i2c_algo_bit drm_kms_helper syscopyarea sysfillrect sysimgblt fb_sys_fops ttm sd_mod crc_t10dif crct10dif_generic crct10dif_pclmul crct10dif_common crc32c_intel serio_raw ata_generic pata_acpi mlx5_core ata_piix tg3 drm devlink libata i2c_core ptp hpsa scsi_transport_sas pps_core dm_mirror dm_region_hash dm_log dm_mod [last unloaded: ib_srpt]
[ 7552.800058] CPU: 7 PID: 1090 Comm: kworker/7:1H Not tainted 3.10.0-768.rt56.699.el7.x86_64 #1
[ 7552.800058] Hardware name: HP ProLiant DL380p Gen8, BIOS P70 11/14/2013
[ 7552.800066] Workqueue: ib-comp-wq ib_cq_poll_work [ib_core]
[ 7552.800067] Call Trace:
[ 7552.800075]  [<ffffffffb76cd055>] dump_stack+0x19/0x1b
[ 7552.800078]  [<ffffffffb70807bb>] __warn+0xfb/0x120
[ 7552.800080]  [<ffffffffb70808fd>] warn_slowpath_null+0x1d/0x20
[ 7552.800085]  [<ffffffffc0ab3983>] __transport_check_aborted_status+0x153/0x190 [target_core_mod]
[ 7552.800091]  [<ffffffffc0ab5c04>] target_execute_cmd+0x34/0x2e0 [target_core_mod]
[ 7552.800096]  [<ffffffffc0ab5fc2>] transport_generic_new_cmd+0x112/0x240 [target_core_mod]
[ 7552.800100]  [<ffffffffc0ab6132>] transport_handle_cdb_direct+0x42/0x90 [target_core_mod]
[ 7552.800105]  [<ffffffffc0ab62cd>] target_submit_cmd_map_sgls+0x14d/0x210 [target_core_mod]
[ 7552.800107]  [<ffffffffc09c15b4>] srpt_handle_new_iu+0x254/0x660 [ib_srpt]
[ 7552.800109]  [<ffffffffc09c1bc8>] srpt_recv_done+0x38/0x60 [ib_srpt]
[ 7552.800113]  [<ffffffffc07a5fb5>] __ib_process_cq+0x65/0xe0 [ib_core]
[ 7552.800118]  [<ffffffffc07a60a0>] ib_cq_poll_work+0x20/0x60 [ib_core]
[ 7552.800120]  [<ffffffffb70a4336>] process_one_work+0x176/0x4a0
[ 7552.800121]  [<ffffffffb70a50ec>] worker_thread+0x16c/0x3f0
[ 7552.800123]  [<ffffffffb70a4f80>] ? manage_workers.isra.36+0x2b0/0x2b0
[ 7552.800125]  [<ffffffffb70ac62f>] kthread+0xcf/0xe0
[ 7552.800139]  [<ffffffffb70ac560>] ? kthread_worker_fn+0x170/0x170
[ 7552.800151]  [<ffffffffb76dd1d8>] ret_from_fork+0x58/0x90
[ 7552.800153]  [<ffffffffb70ac560>] ? kthread_worker_fn+0x170/0x170
[ 7552.800154] ---[ end trace 0000000000000002 ]---
[ 7554.164964] srpt/0xf4521403000e4aa0f4521403000e4ad0: Unsupported SCSI Opcode 0xa3, sending CHECK_CONDITION.
[ 7554.231254] srpt/0xf4521403000e4aa0f4521403000e4ad0: Unsupported SCSI Opcode 0xa3, sending CHECK_CONDITION.
[ 7554.294860] srpt/0xf4521403000e4aa0f4521403000e4ad0: Unsupported SCSI Opcode 0xa3, sending CHECK_CONDITION.
[ 7554.360810] srpt/0xf4521403000e4aa0f4521403000e4ad0: Unsupported SCSI Opcode 0xa3, sending CHECK_CONDITION.
[ 7554.421867] srpt/0xf4521403000e4aa0f4521403000e4ad0: Unsupported SCSI Opcode 0xa3, sending CHECK_CONDITION.
[ 7554.485931] srpt/0xf4521403000e4aa0f4521403000e4ad0: Unsupported SCSI Opcode 0xa3, sending CHECK_CONDITION.
[ 7554.546909] srpt/0xf4521403000e4aa0f4521403000e4ad0: Unsupported SCSI Opcode 0xa3, sending CHECK_CONDITION.
[ 7554.607820] srpt/0xf4521403000e4aa0f4521403000e4ad0: Unsupported SCSI Opcode 0xa3, sending CHECK_CONDITION.
[ 7554.671883] srpt/0xf4521403000e4aa0f4521403000e4ad0: Unsupported SCSI Opcode 0xa3, sending CHECK_CONDITION.
[ 7554.730826] srpt/0xf4521403000e4aa0f4521403000e4ad0: Unsupported SCSI Opcode 0xa3, sending CHECK_CONDITION.

static int __transport_check_aborted_status(struct se_cmd *cmd, int send_status)
        __releases(&cmd->t_state_lock)
        __acquires(&cmd->t_state_lock)
{
        int ret;

        assert_spin_locked(&cmd->t_state_lock);
        WARN_ON_ONCE_NONRT(!irqs_disabled());
<SNIP>

And it is called just from these two places:

int transport_check_aborted_status(struct se_cmd *cmd, int send_status)
{
        int ret;

        spin_lock_irq(&cmd->t_state_lock);
        ret = __transport_check_aborted_status(cmd, send_status);
        spin_unlock_irq(&cmd->t_state_lock);

        return ret;
}
EXPORT_SYMBOL(transport_check_aborted_status);

And:

void target_execute_cmd(struct se_cmd *cmd)
{
        /*
         * Determine if frontend context caller is requesting the stopping of
         * this command for frontend exceptions.
         *
         * If the received CDB has aleady been aborted stop processing it here.
         */
        spin_lock_irq(&cmd->t_state_lock);
        if (__transport_check_aborted_status(cmd, 1)) {
                spin_unlock_irq(&cmd->t_state_lock);
                return;
        }
<SNIP>

Since cmd->t_state_lock becomes a sleeping spin lock on RT, that thing
triggers, turn it into a NONRT WARN_ON, a kernel built with this patch
passes the test case that lead to a BZ being filled for the kernel-rt
package:

https://bugzilla.redhat.com/show_bug.cgi?id=1512875

Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>

---

diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 4558f2e1fe1b..318453e7adfd 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -3204,7 +3204,7 @@ static int __transport_check_aborted_status(struct se_cmd *cmd, int send_status)
 	int ret;
 
 	assert_spin_locked(&cmd->t_state_lock);
-	WARN_ON_ONCE(!irqs_disabled());
+	WARN_ON_ONCE_NONRT(!irqs_disabled());
 
 	if (!(cmd->transport_state & CMD_T_ABORTED))
 		return 0;

[PATCH] target: Use WARNON_NON_RT(!irqs_disabled())

[Arnaldo Carvalho de Melo]

Hi,

	We got a report where this WARN_ON got triggered:

[ 7552.799997] ------------[ cut here ]------------
[ 7552.800016] WARNING: CPU: 7 PID: 1090 at drivers/target/target_core_transport.c:3009 __transport_check_aborted_status+0x153/0x190 [target_core_mod]
[ 7552.800037] Modules linked in: target_core_user uio target_core_pscsi target_core_file target_core_iblock ib_srpt ib_srp scsi_transport_srp scsi_tgt xt_CHECKSUM iptable_mangle ipt_MASQUERADE nf_nat_masquerade_ipv4 iptable_nat nf_nat_ipv4 nf_nat nf_conntrack_ipv4 nf_defrag_ipv4 xt_conntrack nf_conntrack ipt_REJECT nf_reject_ipv4 tun ebtable_filter ebtables ip6table_filter ip6_tables iptable_filter bridge stp llc ib_isert iscsi_target_mod target_core_mod ib_ucm rpcrdma mlx5_ib sunrpc rdma_ucm ib_uverbs ib_iser rdma_cm iw_cm libiscsi ib_umad ib_ipoib scsi_transport_iscsi ib_cm sb_edac intel_powerclamp coretemp intel_rapl iosf_mbi crc32_pclmul ghash_clmulni_intel aesni_intel lrw gf128mul glue_helper ablk_helper cryptd iTCO_wdt iTCO_vendor_support hfi1 ipmi_ssif sg rdmavt ib_core hpilo hpwdt
  pcspkr ipmi_si
[ 7552.800055]  ipmi_devintf ipmi_msghandler wmi acpi_power_meter ioatdma dca shpchp pcc_cpufreq lpc_ich ip_tables xfs libcrc32c mgag200 i2c_algo_bit drm_kms_helper syscopyarea sysfillrect sysimgblt fb_sys_fops ttm sd_mod crc_t10dif crct10dif_generic crct10dif_pclmul crct10dif_common crc32c_intel serio_raw ata_generic pata_acpi mlx5_core ata_piix tg3 drm devlink libata i2c_core ptp hpsa scsi_transport_sas pps_core dm_mirror dm_region_hash dm_log dm_mod [last unloaded: ib_srpt]
[ 7552.800058] CPU: 7 PID: 1090 Comm: kworker/7:1H Not tainted 3.10.0-768.rt56.699.el7.x86_64 #1
[ 7552.800058] Hardware name: HP ProLiant DL380p Gen8, BIOS P70 11/14/2013
[ 7552.800066] Workqueue: ib-comp-wq ib_cq_poll_work [ib_core]
[ 7552.800067] Call Trace:
[ 7552.800075]  [<ffffffffb76cd055>] dump_stack+0x19/0x1b
[ 7552.800078]  [<ffffffffb70807bb>] __warn+0xfb/0x120
[ 7552.800080]  [<ffffffffb70808fd>] warn_slowpath_null+0x1d/0x20
[ 7552.800085]  [<ffffffffc0ab3983>] __transport_check_aborted_status+0x153/0x190 [target_core_mod]
[ 7552.800091]  [<ffffffffc0ab5c04>] target_execute_cmd+0x34/0x2e0 [target_core_mod]
[ 7552.800096]  [<ffffffffc0ab5fc2>] transport_generic_new_cmd+0x112/0x240 [target_core_mod]
[ 7552.800100]  [<ffffffffc0ab6132>] transport_handle_cdb_direct+0x42/0x90 [target_core_mod]
[ 7552.800105]  [<ffffffffc0ab62cd>] target_submit_cmd_map_sgls+0x14d/0x210 [target_core_mod]
[ 7552.800107]  [<ffffffffc09c15b4>] srpt_handle_new_iu+0x254/0x660 [ib_srpt]
[ 7552.800109]  [<ffffffffc09c1bc8>] srpt_recv_done+0x38/0x60 [ib_srpt]
[ 7552.800113]  [<ffffffffc07a5fb5>] __ib_process_cq+0x65/0xe0 [ib_core]
[ 7552.800118]  [<ffffffffc07a60a0>] ib_cq_poll_work+0x20/0x60 [ib_core]
[ 7552.800120]  [<ffffffffb70a4336>] process_one_work+0x176/0x4a0
[ 7552.800121]  [<ffffffffb70a50ec>] worker_thread+0x16c/0x3f0
[ 7552.800123]  [<ffffffffb70a4f80>] ? manage_workers.isra.36+0x2b0/0x2b0
[ 7552.800125]  [<ffffffffb70ac62f>] kthread+0xcf/0xe0
[ 7552.800139]  [<ffffffffb70ac560>] ? kthread_worker_fn+0x170/0x170
[ 7552.800151]  [<ffffffffb76dd1d8>] ret_from_fork+0x58/0x90
[ 7552.800153]  [<ffffffffb70ac560>] ? kthread_worker_fn+0x170/0x170
[ 7552.800154] ---[ end trace 0000000000000002 ]---
[ 7554.164964] srpt/0xf4521403000e4aa0f4521403000e4ad0: Unsupported SCSI Opcode 0xa3, sending CHECK_CONDITION.
[ 7554.231254] srpt/0xf4521403000e4aa0f4521403000e4ad0: Unsupported SCSI Opcode 0xa3, sending CHECK_CONDITION.
[ 7554.294860] srpt/0xf4521403000e4aa0f4521403000e4ad0: Unsupported SCSI Opcode 0xa3, sending CHECK_CONDITION.
[ 7554.360810] srpt/0xf4521403000e4aa0f4521403000e4ad0: Unsupported SCSI Opcode 0xa3, sending CHECK_CONDITION.
[ 7554.421867] srpt/0xf4521403000e4aa0f4521403000e4ad0: Unsupported SCSI Opcode 0xa3, sending CHECK_CONDITION.
[ 7554.485931] srpt/0xf4521403000e4aa0f4521403000e4ad0: Unsupported SCSI Opcode 0xa3, sending CHECK_CONDITION.
[ 7554.546909] srpt/0xf4521403000e4aa0f4521403000e4ad0: Unsupported SCSI Opcode 0xa3, sending CHECK_CONDITION.
[ 7554.607820] srpt/0xf4521403000e4aa0f4521403000e4ad0: Unsupported SCSI Opcode 0xa3, sending CHECK_CONDITION.
[ 7554.671883] srpt/0xf4521403000e4aa0f4521403000e4ad0: Unsupported SCSI Opcode 0xa3, sending CHECK_CONDITION.
[ 7554.730826] srpt/0xf4521403000e4aa0f4521403000e4ad0: Unsupported SCSI Opcode 0xa3, sending CHECK_CONDITION.

static int __transport_check_aborted_status(struct se_cmd *cmd, int send_status)
        __releases(&cmd->t_state_lock)
        __acquires(&cmd->t_state_lock)
{
        int ret;

        assert_spin_locked(&cmd->t_state_lock);
        WARN_ON_ONCE_NONRT(!irqs_disabled());
<SNIP>

And it is called just from these two places:

int transport_check_aborted_status(struct se_cmd *cmd, int send_status)
{
        int ret;

        spin_lock_irq(&cmd->t_state_lock);
        ret = __transport_check_aborted_status(cmd, send_status);
        spin_unlock_irq(&cmd->t_state_lock);

        return ret;
}
EXPORT_SYMBOL(transport_check_aborted_status);

And:

void target_execute_cmd(struct se_cmd *cmd)
{
        /*
         * Determine if frontend context caller is requesting the stopping of
         * this command for frontend exceptions.
         *
         * If the received CDB has aleady been aborted stop processing it here.
         */
        spin_lock_irq(&cmd->t_state_lock);
        if (__transport_check_aborted_status(cmd, 1)) {
                spin_unlock_irq(&cmd->t_state_lock);
                return;
        }
<SNIP>

Since cmd->t_state_lock becomes a sleeping spin lock on RT, that thing
triggers, turn it into a NONRT WARN_ON, a kernel built with this patch
passes the test case that lead to a BZ being filled for the kernel-rt
package:

https://bugzilla.redhat.com/show_bug.cgi?id=1512875

Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>

---

diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 4558f2e1fe1b..318453e7adfd 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -3204,7 +3204,7 @@ static int __transport_check_aborted_status(struct se_cmd *cmd, int send_status)
 	int ret;
 
 	assert_spin_locked(&cmd->t_state_lock);
-	WARN_ON_ONCE(!irqs_disabled());
+	WARN_ON_ONCE_NONRT(!irqs_disabled());
 
 	if (!(cmd->transport_state & CMD_T_ABORTED))
 		return 0;

[PATCH] target: Use WARNON_NON_RT(!irqs_disabled())

[Arnaldo Carvalho de Melo]

Hi,

	We got a report where this WARN_ON got triggered:

[ 7552.799997] ------------[ cut here ]------------
[ 7552.800016] WARNING: CPU: 7 PID: 1090 at drivers/target/target_core_transport.c:3009 __transport_check_aborted_status+0x153/0x190 [target_core_mod]
[ 7552.800037] Modules linked in: target_core_user uio target_core_pscsi target_core_file target_core_iblock ib_srpt ib_srp scsi_transport_srp scsi_tgt xt_CHECKSUM iptable_mangle ipt_MASQUERADE nf_nat_masquerade_ipv4 iptable_nat nf_nat_ipv4 nf_nat nf_conntrack_ipv4 nf_defrag_ipv4 xt_conntrack nf_conntrack ipt_REJECT nf_reject_ipv4 tun ebtable_filter ebtables ip6table_filter ip6_tables iptable_filter bridge stp llc ib_isert iscsi_target_mod target_core_mod ib_ucm rpcrdma mlx5_ib sunrpc rdma_ucm ib_uverbs ib_iser rdma_cm iw_cm libiscsi ib_umad ib_ipoib scsi_transport_iscsi ib_cm sb_edac intel_powerclamp coretemp intel_rapl iosf_mbi crc32_pclmul ghash_clmulni_intel aesni_intel lrw gf128mul glue_helper ablk_helper cryptd iTCO_wdt iTCO_vendor_support hfi1 ipmi_ssif sg rdmavt ib_core hpilo hpwdt pcspkr ipmi_si
[ 7552.800055]  ipmi_devintf ipmi_msghandler wmi acpi_power_meter ioatdma dca shpchp pcc_cpufreq lpc_ich ip_tables xfs libcrc32c mgag200 i2c_algo_bit drm_kms_helper syscopyarea sysfillrect sysimgblt fb_sys_fops ttm sd_mod crc_t10dif crct10dif_generic crct10dif_pclmul crct10dif_common crc32c_intel serio_raw ata_generic pata_acpi mlx5_core ata_piix tg3 drm devlink libata i2c_core ptp hpsa scsi_transport_sas pps_core dm_mirror dm_region_hash dm_log dm_mod [last unloaded: ib_srpt]
[ 7552.800058] CPU: 7 PID: 1090 Comm: kworker/7:1H Not tainted 3.10.0-768.rt56.699.el7.x86_64 #1
[ 7552.800058] Hardware name: HP ProLiant DL380p Gen8, BIOS P70 11/14/2013
[ 7552.800066] Workqueue: ib-comp-wq ib_cq_poll_work [ib_core]
[ 7552.800067] Call Trace:
[ 7552.800075]  [<ffffffffb76cd055>] dump_stack+0x19/0x1b
[ 7552.800078]  [<ffffffffb70807bb>] __warn+0xfb/0x120
[ 7552.800080]  [<ffffffffb70808fd>] warn_slowpath_null+0x1d/0x20
[ 7552.800085]  [<ffffffffc0ab3983>] __transport_check_aborted_status+0x153/0x190 [target_core_mod]
[ 7552.800091]  [<ffffffffc0ab5c04>] target_execute_cmd+0x34/0x2e0 [target_core_mod]
[ 7552.800096]  [<ffffffffc0ab5fc2>] transport_generic_new_cmd+0x112/0x240 [target_core_mod]
[ 7552.800100]  [<ffffffffc0ab6132>] transport_handle_cdb_direct+0x42/0x90 [target_core_mod]
[ 7552.800105]  [<ffffffffc0ab62cd>] target_submit_cmd_map_sgls+0x14d/0x210 [target_core_mod]
[ 7552.800107]  [<ffffffffc09c15b4>] srpt_handle_new_iu+0x254/0x660 [ib_srpt]
[ 7552.800109]  [<ffffffffc09c1bc8>] srpt_recv_done+0x38/0x60 [ib_srpt]
[ 7552.800113]  [<ffffffffc07a5fb5>] __ib_process_cq+0x65/0xe0 [ib_core]
[ 7552.800118]  [<ffffffffc07a60a0>] ib_cq_poll_work+0x20/0x60 [ib_core]
[ 7552.800120]  [<ffffffffb70a4336>] process_one_work+0x176/0x4a0
[ 7552.800121]  [<ffffffffb70a50ec>] worker_thread+0x16c/0x3f0
[ 7552.800123]  [<ffffffffb70a4f80>] ? manage_workers.isra.36+0x2b0/0x2b0
[ 7552.800125]  [<ffffffffb70ac62f>] kthread+0xcf/0xe0
[ 7552.800139]  [<ffffffffb70ac560>] ? kthread_worker_fn+0x170/0x170
[ 7552.800151]  [<ffffffffb76dd1d8>] ret_from_fork+0x58/0x90
[ 7552.800153]  [<ffffffffb70ac560>] ? kthread_worker_fn+0x170/0x170
[ 7552.800154] ---[ end trace 0000000000000002 ]---
[ 7554.164964] srpt/0xf4521403000e4aa0f4521403000e4ad0: Unsupported SCSI Opcode 0xa3, sending CHECK_CONDITION.
[ 7554.231254] srpt/0xf4521403000e4aa0f4521403000e4ad0: Unsupported SCSI Opcode 0xa3, sending CHECK_CONDITION.
[ 7554.294860] srpt/0xf4521403000e4aa0f4521403000e4ad0: Unsupported SCSI Opcode 0xa3, sending CHECK_CONDITION.
[ 7554.360810] srpt/0xf4521403000e4aa0f4521403000e4ad0: Unsupported SCSI Opcode 0xa3, sending CHECK_CONDITION.
[ 7554.421867] srpt/0xf4521403000e4aa0f4521403000e4ad0: Unsupported SCSI Opcode 0xa3, sending CHECK_CONDITION.
[ 7554.485931] srpt/0xf4521403000e4aa0f4521403000e4ad0: Unsupported SCSI Opcode 0xa3, sending CHECK_CONDITION.
[ 7554.546909] srpt/0xf4521403000e4aa0f4521403000e4ad0: Unsupported SCSI Opcode 0xa3, sending CHECK_CONDITION.
[ 7554.607820] srpt/0xf4521403000e4aa0f4521403000e4ad0: Unsupported SCSI Opcode 0xa3, sending CHECK_CONDITION.
[ 7554.671883] srpt/0xf4521403000e4aa0f4521403000e4ad0: Unsupported SCSI Opcode 0xa3, sending CHECK_CONDITION.
[ 7554.730826] srpt/0xf4521403000e4aa0f4521403000e4ad0: Unsupported SCSI Opcode 0xa3, sending CHECK_CONDITION.

static int __transport_check_aborted_status(struct se_cmd *cmd, int send_status)
        __releases(&cmd->t_state_lock)
        __acquires(&cmd->t_state_lock)
{
        int ret;

        assert_spin_locked(&cmd->t_state_lock);
        WARN_ON_ONCE_NONRT(!irqs_disabled());
<SNIP>

And it is called just from these two places:

int transport_check_aborted_status(struct se_cmd *cmd, int send_status)
{
        int ret;

        spin_lock_irq(&cmd->t_state_lock);
        ret = __transport_check_aborted_status(cmd, send_status);
        spin_unlock_irq(&cmd->t_state_lock);

        return ret;
}
EXPORT_SYMBOL(transport_check_aborted_status);

And:

void target_execute_cmd(struct se_cmd *cmd)
{
        /*
         * Determine if frontend context caller is requesting the stopping of
         * this command for frontend exceptions.
         *
         * If the received CDB has aleady been aborted stop processing it here.
         */
        spin_lock_irq(&cmd->t_state_lock);
        if (__transport_check_aborted_status(cmd, 1)) {
                spin_unlock_irq(&cmd->t_state_lock);
                return;
        }
<SNIP>

Since cmd->t_state_lock becomes a sleeping spin lock on RT, that thing
triggers, turn it into a NONRT WARN_ON, a kernel built with this patch
passes the test case that lead to a BZ being filled for the kernel-rt
package:

https://bugzilla.redhat.com/show_bug.cgi?id\x1512875

Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>

---

diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 4558f2e1fe1b..318453e7adfd 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -3204,7 +3204,7 @@ static int __transport_check_aborted_status(struct se_cmd *cmd, int send_status)
 	int ret;
 
 	assert_spin_locked(&cmd->t_state_lock);
-	WARN_ON_ONCE(!irqs_disabled());
+	WARN_ON_ONCE_NONRT(!irqs_disabled());
 
 	if (!(cmd->transport_state & CMD_T_ABORTED))
 		return 0;

Re: [PATCH] target: Use WARNON_NON_RT(!irqs_disabled())

[Linus Torvalds]

[ Adding PeterZ to participants due to query about lockdep_assert() ]

On Wed, Mar 21, 2018 at 8:38 AM, Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
>
>         assert_spin_locked(&cmd->t_state_lock);
> -       WARN_ON_ONCE(!irqs_disabled());
> +       WARN_ON_ONCE_NONRT(!irqs_disabled());

Ugh.

Can't we just replace both of those with a lockdep annotation?

Does "lockdep_assert_held()" already verify the irq contextr, or do we
need lockdep_assert_irqs_disabled() too?

Honestly, the old-fashioned way of doing verification of state by hand
is understandable, but it's legacy and kind of pointless when we have
much better tools these days.

I'm perfectly willing to leave old assertions in place, but if they
need fixing anyway, I'd damn well want to fix them *right* instead of
starting to just add more piles of hacks on top of the old model.

Because when the details of the locking rules depend on RT vs non-RT,
I want the checks to make sense.  And presumably lockdep is the thing
that really knows what the status of a lock is, no?

                  Linus

Re: [PATCH] target: Use WARNON_NON_RT(!irqs_disabled())

[Linus Torvalds]

[ Adding PeterZ to participants due to query about lockdep_assert() ]

On Wed, Mar 21, 2018 at 8:38 AM, Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
>
>         assert_spin_locked(&cmd->t_state_lock);
> -       WARN_ON_ONCE(!irqs_disabled());
> +       WARN_ON_ONCE_NONRT(!irqs_disabled());

Ugh.

Can't we just replace both of those with a lockdep annotation?

Does "lockdep_assert_held()" already verify the irq contextr, or do we
need lockdep_assert_irqs_disabled() too?

Honestly, the old-fashioned way of doing verification of state by hand
is understandable, but it's legacy and kind of pointless when we have
much better tools these days.

I'm perfectly willing to leave old assertions in place, but if they
need fixing anyway, I'd damn well want to fix them *right* instead of
starting to just add more piles of hacks on top of the old model.

Because when the details of the locking rules depend on RT vs non-RT,
I want the checks to make sense.  And presumably lockdep is the thing
that really knows what the status of a lock is, no?

                  Linus

Re: [PATCH] target: Use WARNON_NON_RT(!irqs_disabled())

[Christoph Hellwig]

On Wed, Mar 21, 2018 at 12:38:54PM -0300, Arnaldo Carvalho de Melo wrote:
>  	assert_spin_locked(&cmd->t_state_lock);
> -	WARN_ON_ONCE(!irqs_disabled());
> +	WARN_ON_ONCE_NONRT(!irqs_disabled());

I can't find where WARN_ON_ONCE_NONRT is defined.

That being said I think we can just kill these asserts.  If we have irqs
disabled spin_unlock_irq a few lines below should already warn.

Re: [PATCH] target: Use WARNON_NON_RT(!irqs_disabled())

[Christoph Hellwig]

On Wed, Mar 21, 2018 at 12:38:54PM -0300, Arnaldo Carvalho de Melo wrote:
>  	assert_spin_locked(&cmd->t_state_lock);
> -	WARN_ON_ONCE(!irqs_disabled());
> +	WARN_ON_ONCE_NONRT(!irqs_disabled());

I can't find where WARN_ON_ONCE_NONRT is defined.

That being said I think we can just kill these asserts.  If we have irqs
disabled spin_unlock_irq a few lines below should already warn.

Re: [PATCH] target: Use WARNON_NON_RT(!irqs_disabled())

[Steven Rostedt]

On Wed, 21 Mar 2018 11:50:01 -0700
Christoph Hellwig <hch@infradead.org> wrote:

> On Wed, Mar 21, 2018 at 12:38:54PM -0300, Arnaldo Carvalho de Melo wrote:
> >  	assert_spin_locked(&cmd->t_state_lock);
> > -	WARN_ON_ONCE(!irqs_disabled());
> > +	WARN_ON_ONCE_NONRT(!irqs_disabled());  
> 
> I can't find where WARN_ON_ONCE_NONRT is defined.

It's only in the RT patch set. But that may be changing soon.

> 
> That being said I think we can just kill these asserts.  If we have irqs
> disabled spin_unlock_irq a few lines below should already warn.

I agree with Linus. This should just be some kind of
lockdep_assert_held_irqs_disabeld() or something like that.

-- Steve

Re: [PATCH] target: Use WARNON_NON_RT(!irqs_disabled())

[Steven Rostedt]

On Wed, 21 Mar 2018 11:50:01 -0700
Christoph Hellwig <hch@infradead.org> wrote:

> On Wed, Mar 21, 2018 at 12:38:54PM -0300, Arnaldo Carvalho de Melo wrote:
> >  	assert_spin_locked(&cmd->t_state_lock);
> > -	WARN_ON_ONCE(!irqs_disabled());
> > +	WARN_ON_ONCE_NONRT(!irqs_disabled());  
> 
> I can't find where WARN_ON_ONCE_NONRT is defined.

It's only in the RT patch set. But that may be changing soon.

> 
> That being said I think we can just kill these asserts.  If we have irqs
> disabled spin_unlock_irq a few lines below should already warn.

I agree with Linus. This should just be some kind of
lockdep_assert_held_irqs_disabeld() or something like that.

-- Steve

Re: [PATCH] target: Use WARNON_NON_RT(!irqs_disabled())

[Thomas Gleixner]

On Wed, 21 Mar 2018, Linus Torvalds wrote:
> [ Adding PeterZ to participants due to query about lockdep_assert() ]
> 
> On Wed, Mar 21, 2018 at 8:38 AM, Arnaldo Carvalho de Melo
> <acme@kernel.org> wrote:
> >
> >         assert_spin_locked(&cmd->t_state_lock);
> > -       WARN_ON_ONCE(!irqs_disabled());
> > +       WARN_ON_ONCE_NONRT(!irqs_disabled());
> 
> Ugh.
> 
> Can't we just replace both of those with a lockdep annotation?
> 
> Does "lockdep_assert_held()" already verify the irq contextr, or do we
> need lockdep_assert_irqs_disabled() too?
> 
> Honestly, the old-fashioned way of doing verification of state by hand
> is understandable, but it's legacy and kind of pointless when we have
> much better tools these days.
> 
> I'm perfectly willing to leave old assertions in place, but if they
> need fixing anyway, I'd damn well want to fix them *right* instead of
> starting to just add more piles of hacks on top of the old model.
> 
> Because when the details of the locking rules depend on RT vs non-RT,
> I want the checks to make sense.  And presumably lockdep is the thing
> that really knows what the status of a lock is, no?

We are working on replacing the _NONRT _RT variants with proper lockdep
mechnisms which are aware of the RT vs. non-RT magic under the hood. Just
not there yet.

Thanks,

	tglx

Re: [PATCH] target: Use WARNON_NON_RT(!irqs_disabled())

[Thomas Gleixner]

On Wed, 21 Mar 2018, Linus Torvalds wrote:
> [ Adding PeterZ to participants due to query about lockdep_assert() ]
> 
> On Wed, Mar 21, 2018 at 8:38 AM, Arnaldo Carvalho de Melo
> <acme@kernel.org> wrote:
> >
> >         assert_spin_locked(&cmd->t_state_lock);
> > -       WARN_ON_ONCE(!irqs_disabled());
> > +       WARN_ON_ONCE_NONRT(!irqs_disabled());
> 
> Ugh.
> 
> Can't we just replace both of those with a lockdep annotation?
> 
> Does "lockdep_assert_held()" already verify the irq contextr, or do we
> need lockdep_assert_irqs_disabled() too?
> 
> Honestly, the old-fashioned way of doing verification of state by hand
> is understandable, but it's legacy and kind of pointless when we have
> much better tools these days.
> 
> I'm perfectly willing to leave old assertions in place, but if they
> need fixing anyway, I'd damn well want to fix them *right* instead of
> starting to just add more piles of hacks on top of the old model.
> 
> Because when the details of the locking rules depend on RT vs non-RT,
> I want the checks to make sense.  And presumably lockdep is the thing
> that really knows what the status of a lock is, no?

We are working on replacing the _NONRT _RT variants with proper lockdep
mechnisms which are aware of the RT vs. non-RT magic under the hood. Just
not there yet.

Thanks,

	tglx

Re: [PATCH] target: Use WARNON_NON_RT(!irqs_disabled())

[Arnaldo Carvalho de Melo]

Em Wed, Mar 21, 2018 at 11:43:58AM -0700, Linus Torvalds escreveu:
> [ Adding PeterZ to participants due to query about lockdep_assert() ]
> 
> On Wed, Mar 21, 2018 at 8:38 AM, Arnaldo Carvalho de Melo
> <acme@kernel.org> wrote:
> >
> >         assert_spin_locked(&cmd->t_state_lock);
> > -       WARN_ON_ONCE(!irqs_disabled());
> > +       WARN_ON_ONCE_NONRT(!irqs_disabled());
> 
> Ugh.
> 
> Can't we just replace both of those with a lockdep annotation?

Huh, even better, when that feature gets finished (tglx said it was
being developed, not there yet tho) it'll allow further reduction of the
PREEMPT_RT_FULL patchkit.

- Arnaldo

Re: [PATCH] target: Use WARNON_NON_RT(!irqs_disabled())

[Arnaldo Carvalho de Melo]

Em Wed, Mar 21, 2018 at 11:43:58AM -0700, Linus Torvalds escreveu:
> [ Adding PeterZ to participants due to query about lockdep_assert() ]
> 
> On Wed, Mar 21, 2018 at 8:38 AM, Arnaldo Carvalho de Melo
> <acme@kernel.org> wrote:
> >
> >         assert_spin_locked(&cmd->t_state_lock);
> > -       WARN_ON_ONCE(!irqs_disabled());
> > +       WARN_ON_ONCE_NONRT(!irqs_disabled());
> 
> Ugh.
> 
> Can't we just replace both of those with a lockdep annotation?

Huh, even better, when that feature gets finished (tglx said it was
being developed, not there yet tho) it'll allow further reduction of the
PREEMPT_RT_FULL patchkit.

- Arnaldo

Re: [PATCH] target: Use WARNON_NON_RT(!irqs_disabled())

[Arnaldo Carvalho de Melo]

Em Thu, Mar 22, 2018 at 06:37:45AM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Wed, Mar 21, 2018 at 11:43:58AM -0700, Linus Torvalds escreveu:
> > [ Adding PeterZ to participants due to query about lockdep_assert() ]
> > 
> > On Wed, Mar 21, 2018 at 8:38 AM, Arnaldo Carvalho de Melo
> > <acme@kernel.org> wrote:
> > >
> > >         assert_spin_locked(&cmd->t_state_lock);
> > > -       WARN_ON_ONCE(!irqs_disabled());
> > > +       WARN_ON_ONCE_NONRT(!irqs_disabled());
> > 
> > Ugh.
> > 
> > Can't we just replace both of those with a lockdep annotation?
> 
> Huh, even better, when that feature gets finished (tglx said it was
> being developed, not there yet tho) it'll allow further reduction of the
> PREEMPT_RT_FULL patchkit.

We'd get rid of these:

[acme@jouet patches-4.11.12-rt15]$ grep "^+[[:space:]]\+.*NONRT" *.patch
dm-make-rt-aware.patch:+		BUG_ON_NONRT(!irqs_disabled());
fs-block-rt-support.patch:+	WARN_ON_NONRT(!irqs_disabled());
i915-bogus-warning-from-i915-when-running-on-PREEMPT.patch:+	WARN_ON_NONRT(!in_interrupt());
iommu-amd--Use-WARN_ON_NORT.patch:+	WARN_ON_NONRT(!irqs_disabled());
iommu-amd--Use-WARN_ON_NORT.patch:+	WARN_ON_NONRT(!irqs_disabled());
irqwork-push_most_work_into_softirq_context.patch:+	BUG_ON_NONRT(!irqs_disabled());
net-wireless-warn-nort.patch:+	WARN_ON_ONCE_NONRT(softirq_count() == 0);
posix-timers-thread-posix-cpu-timers-on-rt.patch:+	WARN_ON_ONCE_NONRT(!irqs_disabled());
posix-timers-thread-posix-cpu-timers-on-rt.patch:+	WARN_ON_ONCE_NONRT(!irqs_disabled());
posix-timers-thread-posix-cpu-timers-on-rt.patch:+	WARN_ON_ONCE_NONRT(!irqs_disabled());
workqueue-use-locallock.patch:+	WARN_ON_ONCE_NONRT(!irqs_disabled());
[acme@jouet patches-4.11.12-rt15]$

- Arnaldo

Re: [PATCH] target: Use WARNON_NON_RT(!irqs_disabled())

[Arnaldo Carvalho de Melo]

Em Thu, Mar 22, 2018 at 06:37:45AM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Wed, Mar 21, 2018 at 11:43:58AM -0700, Linus Torvalds escreveu:
> > [ Adding PeterZ to participants due to query about lockdep_assert() ]
> > 
> > On Wed, Mar 21, 2018 at 8:38 AM, Arnaldo Carvalho de Melo
> > <acme@kernel.org> wrote:
> > >
> > >         assert_spin_locked(&cmd->t_state_lock);
> > > -       WARN_ON_ONCE(!irqs_disabled());
> > > +       WARN_ON_ONCE_NONRT(!irqs_disabled());
> > 
> > Ugh.
> > 
> > Can't we just replace both of those with a lockdep annotation?
> 
> Huh, even better, when that feature gets finished (tglx said it was
> being developed, not there yet tho) it'll allow further reduction of the
> PREEMPT_RT_FULL patchkit.

We'd get rid of these:

[acme@jouet patches-4.11.12-rt15]$ grep "^+[[:space:]]\+.*NONRT" *.patch
dm-make-rt-aware.patch:+		BUG_ON_NONRT(!irqs_disabled());
fs-block-rt-support.patch:+	WARN_ON_NONRT(!irqs_disabled());
i915-bogus-warning-from-i915-when-running-on-PREEMPT.patch:+	WARN_ON_NONRT(!in_interrupt());
iommu-amd--Use-WARN_ON_NORT.patch:+	WARN_ON_NONRT(!irqs_disabled());
iommu-amd--Use-WARN_ON_NORT.patch:+	WARN_ON_NONRT(!irqs_disabled());
irqwork-push_most_work_into_softirq_context.patch:+	BUG_ON_NONRT(!irqs_disabled());
net-wireless-warn-nort.patch:+	WARN_ON_ONCE_NONRT(softirq_count() = 0);
posix-timers-thread-posix-cpu-timers-on-rt.patch:+	WARN_ON_ONCE_NONRT(!irqs_disabled());
posix-timers-thread-posix-cpu-timers-on-rt.patch:+	WARN_ON_ONCE_NONRT(!irqs_disabled());
posix-timers-thread-posix-cpu-timers-on-rt.patch:+	WARN_ON_ONCE_NONRT(!irqs_disabled());
workqueue-use-locallock.patch:+	WARN_ON_ONCE_NONRT(!irqs_disabled());
[acme@jouet patches-4.11.12-rt15]$

- Arnaldo

Re: [PATCH] target: Use WARNON_NON_RT(!irqs_disabled())

[Thomas Gleixner]

On Thu, 22 Mar 2018, Arnaldo Carvalho de Melo wrote:
> Em Wed, Mar 21, 2018 at 11:43:58AM -0700, Linus Torvalds escreveu:
> > [ Adding PeterZ to participants due to query about lockdep_assert() ]
> > 
> > On Wed, Mar 21, 2018 at 8:38 AM, Arnaldo Carvalho de Melo
> > <acme@kernel.org> wrote:
> > >
> > >         assert_spin_locked(&cmd->t_state_lock);
> > > -       WARN_ON_ONCE(!irqs_disabled());
> > > +       WARN_ON_ONCE_NONRT(!irqs_disabled());
> > 
> > Ugh.
> > 
> > Can't we just replace both of those with a lockdep annotation?
> 
> Huh, even better, when that feature gets finished (tglx said it was
> being developed, not there yet tho) it'll allow further reduction of the
> PREEMPT_RT_FULL patchkit.

That's the evil plan :)

Re: [PATCH] target: Use WARNON_NON_RT(!irqs_disabled())

[Thomas Gleixner]

On Thu, 22 Mar 2018, Arnaldo Carvalho de Melo wrote:
> Em Wed, Mar 21, 2018 at 11:43:58AM -0700, Linus Torvalds escreveu:
> > [ Adding PeterZ to participants due to query about lockdep_assert() ]
> > 
> > On Wed, Mar 21, 2018 at 8:38 AM, Arnaldo Carvalho de Melo
> > <acme@kernel.org> wrote:
> > >
> > >         assert_spin_locked(&cmd->t_state_lock);
> > > -       WARN_ON_ONCE(!irqs_disabled());
> > > +       WARN_ON_ONCE_NONRT(!irqs_disabled());
> > 
> > Ugh.
> > 
> > Can't we just replace both of those with a lockdep annotation?
> 
> Huh, even better, when that feature gets finished (tglx said it was
> being developed, not there yet tho) it'll allow further reduction of the
> PREEMPT_RT_FULL patchkit.

That's the evil plan :)

Re: [PATCH] target: Use WARNON_NON_RT(!irqs_disabled())

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2503 bytes --]

Hi Arnaldo,

I love your patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v4.16-rc6 next-20180322]
[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/Arnaldo-Carvalho-de-Melo/target-Use-WARNON_NON_RT-irqs_disabled/20180322-174549
config: i386-randconfig-x015-201811 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

   drivers//target/target_core_transport.c: In function '__transport_check_aborted_status':
>> drivers//target/target_core_transport.c:3207:2: error: implicit declaration of function 'WARN_ON_ONCE_NONRT'; did you mean 'WARN_ON_ONCE'? [-Werror=implicit-function-declaration]
     WARN_ON_ONCE_NONRT(!irqs_disabled());
     ^~~~~~~~~~~~~~~~~~
     WARN_ON_ONCE
   cc1: some warnings being treated as errors

vim +3207 drivers//target/target_core_transport.c

  3199	
  3200	static int __transport_check_aborted_status(struct se_cmd *cmd, int send_status)
  3201		__releases(&cmd->t_state_lock)
  3202		__acquires(&cmd->t_state_lock)
  3203	{
  3204		int ret;
  3205	
  3206		assert_spin_locked(&cmd->t_state_lock);
> 3207		WARN_ON_ONCE_NONRT(!irqs_disabled());
  3208	
  3209		if (!(cmd->transport_state & CMD_T_ABORTED))
  3210			return 0;
  3211		/*
  3212		 * If cmd has been aborted but either no status is to be sent or it has
  3213		 * already been sent, just return
  3214		 */
  3215		if (!send_status || !(cmd->se_cmd_flags & SCF_SEND_DELAYED_TAS)) {
  3216			if (send_status)
  3217				cmd->se_cmd_flags |= SCF_SEND_DELAYED_TAS;
  3218			return 1;
  3219		}
  3220	
  3221		pr_debug("Sending delayed SAM_STAT_TASK_ABORTED status for CDB:"
  3222			" 0x%02x ITT: 0x%08llx\n", cmd->t_task_cdb[0], cmd->tag);
  3223	
  3224		cmd->se_cmd_flags &= ~SCF_SEND_DELAYED_TAS;
  3225		cmd->scsi_status = SAM_STAT_TASK_ABORTED;
  3226		trace_target_cmd_complete(cmd);
  3227	
  3228		spin_unlock_irq(&cmd->t_state_lock);
  3229		ret = cmd->se_tfo->queue_status(cmd);
  3230		if (ret)
  3231			transport_handle_queue_full(cmd, cmd->se_dev, ret, false);
  3232		spin_lock_irq(&cmd->t_state_lock);
  3233	
  3234		return 1;
  3235	}
  3236	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 24987 bytes --]

Re: [PATCH] target: Use WARNON_NON_RT(!irqs_disabled())

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2503 bytes --]

Hi Arnaldo,

I love your patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v4.16-rc6 next-20180322]
[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/Arnaldo-Carvalho-de-Melo/target-Use-WARNON_NON_RT-irqs_disabled/20180322-174549
config: i386-randconfig-x015-201811 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

   drivers//target/target_core_transport.c: In function '__transport_check_aborted_status':
>> drivers//target/target_core_transport.c:3207:2: error: implicit declaration of function 'WARN_ON_ONCE_NONRT'; did you mean 'WARN_ON_ONCE'? [-Werror=implicit-function-declaration]
     WARN_ON_ONCE_NONRT(!irqs_disabled());
     ^~~~~~~~~~~~~~~~~~
     WARN_ON_ONCE
   cc1: some warnings being treated as errors

vim +3207 drivers//target/target_core_transport.c

  3199	
  3200	static int __transport_check_aborted_status(struct se_cmd *cmd, int send_status)
  3201		__releases(&cmd->t_state_lock)
  3202		__acquires(&cmd->t_state_lock)
  3203	{
  3204		int ret;
  3205	
  3206		assert_spin_locked(&cmd->t_state_lock);
> 3207		WARN_ON_ONCE_NONRT(!irqs_disabled());
  3208	
  3209		if (!(cmd->transport_state & CMD_T_ABORTED))
  3210			return 0;
  3211		/*
  3212		 * If cmd has been aborted but either no status is to be sent or it has
  3213		 * already been sent, just return
  3214		 */
  3215		if (!send_status || !(cmd->se_cmd_flags & SCF_SEND_DELAYED_TAS)) {
  3216			if (send_status)
  3217				cmd->se_cmd_flags |= SCF_SEND_DELAYED_TAS;
  3218			return 1;
  3219		}
  3220	
  3221		pr_debug("Sending delayed SAM_STAT_TASK_ABORTED status for CDB:"
  3222			" 0x%02x ITT: 0x%08llx\n", cmd->t_task_cdb[0], cmd->tag);
  3223	
  3224		cmd->se_cmd_flags &= ~SCF_SEND_DELAYED_TAS;
  3225		cmd->scsi_status = SAM_STAT_TASK_ABORTED;
  3226		trace_target_cmd_complete(cmd);
  3227	
  3228		spin_unlock_irq(&cmd->t_state_lock);
  3229		ret = cmd->se_tfo->queue_status(cmd);
  3230		if (ret)
  3231			transport_handle_queue_full(cmd, cmd->se_dev, ret, false);
  3232		spin_lock_irq(&cmd->t_state_lock);
  3233	
  3234		return 1;
  3235	}
  3236	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 24987 bytes --]

Re: [PATCH] target: Use WARNON_NON_RT(!irqs_disabled())

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2482 bytes --]

Hi Arnaldo,

I love your patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v4.16-rc6 next-20180322]
[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/Arnaldo-Carvalho-de-Melo/target-Use-WARNON_NON_RT-irqs_disabled/20180322-174549
config: i386-randconfig-s1-03221113 (attached as .config)
compiler: gcc-6 (Debian 6.4.0-9) 6.4.0 20171026
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

   drivers/target/target_core_transport.c: In function '__transport_check_aborted_status':
>> drivers/target/target_core_transport.c:3207:2: error: implicit declaration of function 'WARN_ON_ONCE_NONRT' [-Werror=implicit-function-declaration]
     WARN_ON_ONCE_NONRT(!irqs_disabled());
     ^~~~~~~~~~~~~~~~~~
   cc1: some warnings being treated as errors

vim +/WARN_ON_ONCE_NONRT +3207 drivers/target/target_core_transport.c

  3199	
  3200	static int __transport_check_aborted_status(struct se_cmd *cmd, int send_status)
  3201		__releases(&cmd->t_state_lock)
  3202		__acquires(&cmd->t_state_lock)
  3203	{
  3204		int ret;
  3205	
  3206		assert_spin_locked(&cmd->t_state_lock);
> 3207		WARN_ON_ONCE_NONRT(!irqs_disabled());
  3208	
  3209		if (!(cmd->transport_state & CMD_T_ABORTED))
  3210			return 0;
  3211		/*
  3212		 * If cmd has been aborted but either no status is to be sent or it has
  3213		 * already been sent, just return
  3214		 */
  3215		if (!send_status || !(cmd->se_cmd_flags & SCF_SEND_DELAYED_TAS)) {
  3216			if (send_status)
  3217				cmd->se_cmd_flags |= SCF_SEND_DELAYED_TAS;
  3218			return 1;
  3219		}
  3220	
  3221		pr_debug("Sending delayed SAM_STAT_TASK_ABORTED status for CDB:"
  3222			" 0x%02x ITT: 0x%08llx\n", cmd->t_task_cdb[0], cmd->tag);
  3223	
  3224		cmd->se_cmd_flags &= ~SCF_SEND_DELAYED_TAS;
  3225		cmd->scsi_status = SAM_STAT_TASK_ABORTED;
  3226		trace_target_cmd_complete(cmd);
  3227	
  3228		spin_unlock_irq(&cmd->t_state_lock);
  3229		ret = cmd->se_tfo->queue_status(cmd);
  3230		if (ret)
  3231			transport_handle_queue_full(cmd, cmd->se_dev, ret, false);
  3232		spin_lock_irq(&cmd->t_state_lock);
  3233	
  3234		return 1;
  3235	}
  3236	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 28546 bytes --]

Re: [PATCH] target: Use WARNON_NON_RT(!irqs_disabled())

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2482 bytes --]

Hi Arnaldo,

I love your patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v4.16-rc6 next-20180322]
[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/Arnaldo-Carvalho-de-Melo/target-Use-WARNON_NON_RT-irqs_disabled/20180322-174549
config: i386-randconfig-s1-03221113 (attached as .config)
compiler: gcc-6 (Debian 6.4.0-9) 6.4.0 20171026
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

   drivers/target/target_core_transport.c: In function '__transport_check_aborted_status':
>> drivers/target/target_core_transport.c:3207:2: error: implicit declaration of function 'WARN_ON_ONCE_NONRT' [-Werror=implicit-function-declaration]
     WARN_ON_ONCE_NONRT(!irqs_disabled());
     ^~~~~~~~~~~~~~~~~~
   cc1: some warnings being treated as errors

vim +/WARN_ON_ONCE_NONRT +3207 drivers/target/target_core_transport.c

  3199	
  3200	static int __transport_check_aborted_status(struct se_cmd *cmd, int send_status)
  3201		__releases(&cmd->t_state_lock)
  3202		__acquires(&cmd->t_state_lock)
  3203	{
  3204		int ret;
  3205	
  3206		assert_spin_locked(&cmd->t_state_lock);
> 3207		WARN_ON_ONCE_NONRT(!irqs_disabled());
  3208	
  3209		if (!(cmd->transport_state & CMD_T_ABORTED))
  3210			return 0;
  3211		/*
  3212		 * If cmd has been aborted but either no status is to be sent or it has
  3213		 * already been sent, just return
  3214		 */
  3215		if (!send_status || !(cmd->se_cmd_flags & SCF_SEND_DELAYED_TAS)) {
  3216			if (send_status)
  3217				cmd->se_cmd_flags |= SCF_SEND_DELAYED_TAS;
  3218			return 1;
  3219		}
  3220	
  3221		pr_debug("Sending delayed SAM_STAT_TASK_ABORTED status for CDB:"
  3222			" 0x%02x ITT: 0x%08llx\n", cmd->t_task_cdb[0], cmd->tag);
  3223	
  3224		cmd->se_cmd_flags &= ~SCF_SEND_DELAYED_TAS;
  3225		cmd->scsi_status = SAM_STAT_TASK_ABORTED;
  3226		trace_target_cmd_complete(cmd);
  3227	
  3228		spin_unlock_irq(&cmd->t_state_lock);
  3229		ret = cmd->se_tfo->queue_status(cmd);
  3230		if (ret)
  3231			transport_handle_queue_full(cmd, cmd->se_dev, ret, false);
  3232		spin_lock_irq(&cmd->t_state_lock);
  3233	
  3234		return 1;
  3235	}
  3236	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 28546 bytes --]

Re: [PATCH] target: Use WARNON_NON_RT(!irqs_disabled())

[Sebastian Andrzej Siewior]

On 2018-03-22 06:37:45 [-0300], Arnaldo Carvalho de Melo wrote:
> Em Wed, Mar 21, 2018 at 11:43:58AM -0700, Linus Torvalds escreveu:
> > [ Adding PeterZ to participants due to query about lockdep_assert() ]
> > 
> > On Wed, Mar 21, 2018 at 8:38 AM, Arnaldo Carvalho de Melo
> > <acme@kernel.org> wrote:
> > >
> > >         assert_spin_locked(&cmd->t_state_lock);
> > > -       WARN_ON_ONCE(!irqs_disabled());
> > > +       WARN_ON_ONCE_NONRT(!irqs_disabled());
> > 
> > Ugh.
> > 
> > Can't we just replace both of those with a lockdep annotation?
> 
> Huh, even better, when that feature gets finished (tglx said it was
> being developed, not there yet tho) it'll allow further reduction of the
> PREEMPT_RT_FULL patchkit.

I am going take this into -RT tree for now until we have different
solution. I will try to be kind and do the same change in
__transport_wait_for_tasks().
Arnaldo, please do "[PATCH RT]" while sending patches. Then the bots
don't complain if it applies but does not compile on !RT kernel (or so
I've been told).

Technically speaking the code wants to ensure that the lock is held and
the interrupts are disabled because the lock is always taken with
disabled interrupts. This kind of check could be done with
	lockdep_assert_held(&cmd->t_state_lock);

but would require lockdep to be switched on. Nicholas, would you mind
such a change?

> - Arnaldo

Sebastian

Re: [PATCH] target: Use WARNON_NON_RT(!irqs_disabled())

[Sebastian Andrzej Siewior]

On 2018-03-22 06:37:45 [-0300], Arnaldo Carvalho de Melo wrote:
> Em Wed, Mar 21, 2018 at 11:43:58AM -0700, Linus Torvalds escreveu:
> > [ Adding PeterZ to participants due to query about lockdep_assert() ]
> > 
> > On Wed, Mar 21, 2018 at 8:38 AM, Arnaldo Carvalho de Melo
> > <acme@kernel.org> wrote:
> > >
> > >         assert_spin_locked(&cmd->t_state_lock);
> > > -       WARN_ON_ONCE(!irqs_disabled());
> > > +       WARN_ON_ONCE_NONRT(!irqs_disabled());
> > 
> > Ugh.
> > 
> > Can't we just replace both of those with a lockdep annotation?
> 
> Huh, even better, when that feature gets finished (tglx said it was
> being developed, not there yet tho) it'll allow further reduction of the
> PREEMPT_RT_FULL patchkit.

I am going take this into -RT tree for now until we have different
solution. I will try to be kind and do the same change in
__transport_wait_for_tasks().
Arnaldo, please do "[PATCH RT]" while sending patches. Then the bots
don't complain if it applies but does not compile on !RT kernel (or so
I've been told).

Technically speaking the code wants to ensure that the lock is held and
the interrupts are disabled because the lock is always taken with
disabled interrupts. This kind of check could be done with
	lockdep_assert_held(&cmd->t_state_lock);

but would require lockdep to be switched on. Nicholas, would you mind
such a change?

> - Arnaldo

Sebastian

Re: [PATCH] target: Use WARNON_NON_RT(!irqs_disabled())

[Bart Van Assche]

On Fri, 2018-03-23 at 16:55 +0100, Sebastian Andrzej Siewior wrote:
> I am going take this into -RT tree for now until we have different
> solution.

Have you considered to delete the WARN_ON_ONCE(!irqs_disabled()) statement?
I think that check duplicates functionality that already exists in lockdep
since lockdep is already able to detect spinlock use inconsistencies.

Bart.

Re: [PATCH] target: Use WARNON_NON_RT(!irqs_disabled())

[Bart Van Assche]

T24gRnJpLCAyMDE4LTAzLTIzIGF0IDE2OjU1ICswMTAwLCBTZWJhc3RpYW4gQW5kcnplaiBTaWV3
aW9yIHdyb3RlOg0KPiBJIGFtIGdvaW5nIHRha2UgdGhpcyBpbnRvIC1SVCB0cmVlIGZvciBub3cg
dW50aWwgd2UgaGF2ZSBkaWZmZXJlbnQNCj4gc29sdXRpb24uDQoNCkhhdmUgeW91IGNvbnNpZGVy
ZWQgdG8gZGVsZXRlIHRoZSBXQVJOX09OX09OQ0UoIWlycXNfZGlzYWJsZWQoKSkgc3RhdGVtZW50
Pw0KSSB0aGluayB0aGF0IGNoZWNrIGR1cGxpY2F0ZXMgZnVuY3Rpb25hbGl0eSB0aGF0IGFscmVh
ZHkgZXhpc3RzIGluIGxvY2tkZXANCnNpbmNlIGxvY2tkZXAgaXMgYWxyZWFkeSBhYmxlIHRvIGRl
dGVjdCBzcGlubG9jayB1c2UgaW5jb25zaXN0ZW5jaWVzLg0KDQpCYXJ0Lg0KDQoNCg=

Re: [PATCH] target: Use WARNON_NON_RT(!irqs_disabled())

[bigeasy]

On 2018-03-23 16:25:25 [+0000], Bart Van Assche wrote:
> On Fri, 2018-03-23 at 16:55 +0100, Sebastian Andrzej Siewior wrote:
> > I am going take this into -RT tree for now until we have different
> > solution.
> 
> Have you considered to delete the WARN_ON_ONCE(!irqs_disabled()) statement?
> I think that check duplicates functionality that already exists in lockdep
> since lockdep is already able to detect spinlock use inconsistencies.

correct. That is why I suggested to use lockdep_assert_held() instead of
this IRQ-check + the spin_lock_assert().
The only downside is that this code (as of now) works with lockdep
disabled. On the other hand, lockdep_assert_held() gives you a splat
instead of a BUG() statement like spin_lock_assert() does so I clearly
promote lockdep here :)

> Bart.

Sebastian

Re: [PATCH] target: Use WARNON_NON_RT(!irqs_disabled())

[bigeasy]

On 2018-03-23 16:25:25 [+0000], Bart Van Assche wrote:
> On Fri, 2018-03-23 at 16:55 +0100, Sebastian Andrzej Siewior wrote:
> > I am going take this into -RT tree for now until we have different
> > solution.
> 
> Have you considered to delete the WARN_ON_ONCE(!irqs_disabled()) statement?
> I think that check duplicates functionality that already exists in lockdep
> since lockdep is already able to detect spinlock use inconsistencies.

correct. That is why I suggested to use lockdep_assert_held() instead of
this IRQ-check + the spin_lock_assert().
The only downside is that this code (as of now) works with lockdep
disabled. On the other hand, lockdep_assert_held() gives you a splat
instead of a BUG() statement like spin_lock_assert() does so I clearly
promote lockdep here :)

> Bart.

Sebastian

Re: [PATCH] target: Use WARNON_NON_RT(!irqs_disabled())

[Linus Torvalds]

On Fri, Mar 23, 2018 at 9:25 AM, Bart Van Assche <Bart.VanAssche@wdc.com> wrote:
>
> Have you considered to delete the WARN_ON_ONCE(!irqs_disabled()) statement?
> I think that check duplicates functionality that already exists in lockdep
> since lockdep is already able to detect spinlock use inconsistencies.

Please just delete both lines.

There is exactly two callers of that static function, and both of them do

        spin_lock_irq(&cmd->t_state_lock);

right above the call.

It's not like this is some function that is exported to random users,
and we should check that the calling convention is right.

So honestly, even lockdep annotations look like you don't need them.

This looks like "it may have been useful during coding to document
things, but it's not useful long-term".

Sure, the annotation is not wrong, but even if you go "verification is
good", you should ask yourself whether there are maybe better places
that would catch more relevant problems, than putting verification
into some static function with two trivially correct callers wrt this
verification?

              Linus

Re: [PATCH] target: Use WARNON_NON_RT(!irqs_disabled())

[Linus Torvalds]

On Fri, Mar 23, 2018 at 9:25 AM, Bart Van Assche <Bart.VanAssche@wdc.com> wrote:
>
> Have you considered to delete the WARN_ON_ONCE(!irqs_disabled()) statement?
> I think that check duplicates functionality that already exists in lockdep
> since lockdep is already able to detect spinlock use inconsistencies.

Please just delete both lines.

There is exactly two callers of that static function, and both of them do

        spin_lock_irq(&cmd->t_state_lock);

right above the call.

It's not like this is some function that is exported to random users,
and we should check that the calling convention is right.

So honestly, even lockdep annotations look like you don't need them.

This looks like "it may have been useful during coding to document
things, but it's not useful long-term".

Sure, the annotation is not wrong, but even if you go "verification is
good", you should ask yourself whether there are maybe better places
that would catch more relevant problems, than putting verification
into some static function with two trivially correct callers wrt this
verification?

              Linus

[PATCH 1/2] target: drop spin_lock_assert() + irqs_disabled() combo checks

[bigeasy]

There are a few functions which check for if the lock is held
(spin_lock_assert()) and the interrupts are disabled (irqs_disabled()).
>From looking at the code, each function is static, the caller is near by
and does spin_lock_irq|safe(). As Linus puts it:

|It's not like this is some function that is exported to random users,
|and we should check that the calling convention is right.
|
|This looks like "it may have been useful during coding to document
|things, but it's not useful long-term".

Remove those checks.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/target/target_core_tmr.c       | 2 --
 drivers/target/target_core_transport.c | 6 ------
 2 files changed, 8 deletions(-)

diff --git a/drivers/target/target_core_tmr.c b/drivers/target/target_core_tmr.c
index 9c7bc1ca341a..3d35dad1de2c 100644
--- a/drivers/target/target_core_tmr.c
+++ b/drivers/target/target_core_tmr.c
@@ -114,8 +114,6 @@ static bool __target_check_io_state(struct se_cmd *se_cmd,
 {
 	struct se_session *sess = se_cmd->se_sess;
 
-	assert_spin_locked(&sess->sess_cmd_lock);
-	WARN_ON_ONCE(!irqs_disabled());
 	/*
 	 * If command already reached CMD_T_COMPLETE state within
 	 * target_complete_cmd() or CMD_T_FABRIC_STOP due to shutdown,
diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 74b646f165d4..2c3ddb3a4124 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -2954,9 +2954,6 @@ __transport_wait_for_tasks(struct se_cmd *cmd, bool fabric_stop,
 	__acquires(&cmd->t_state_lock)
 {
 
-	assert_spin_locked(&cmd->t_state_lock);
-	WARN_ON_ONCE(!irqs_disabled());
-
 	if (fabric_stop)
 		cmd->transport_state |= CMD_T_FABRIC_STOP;
 
@@ -3241,9 +3238,6 @@ static int __transport_check_aborted_status(struct se_cmd *cmd, int send_status)
 {
 	int ret;
 
-	assert_spin_locked(&cmd->t_state_lock);
-	WARN_ON_ONCE(!irqs_disabled());
-
 	if (!(cmd->transport_state & CMD_T_ABORTED))
 		return 0;
 	/*
-- 
2.16.2

[PATCH 1/2] target: drop spin_lock_assert() + irqs_disabled() combo checks

[bigeasy]

There are a few functions which check for if the lock is held
(spin_lock_assert()) and the interrupts are disabled (irqs_disabled()).
From looking at the code, each function is static, the caller is near by
and does spin_lock_irq|safe(). As Linus puts it:

|It's not like this is some function that is exported to random users,
|and we should check that the calling convention is right.
|
|This looks like "it may have been useful during coding to document
|things, but it's not useful long-term".

Remove those checks.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/target/target_core_tmr.c       | 2 --
 drivers/target/target_core_transport.c | 6 ------
 2 files changed, 8 deletions(-)

diff --git a/drivers/target/target_core_tmr.c b/drivers/target/target_core_tmr.c
index 9c7bc1ca341a..3d35dad1de2c 100644
--- a/drivers/target/target_core_tmr.c
+++ b/drivers/target/target_core_tmr.c
@@ -114,8 +114,6 @@ static bool __target_check_io_state(struct se_cmd *se_cmd,
 {
 	struct se_session *sess = se_cmd->se_sess;
 
-	assert_spin_locked(&sess->sess_cmd_lock);
-	WARN_ON_ONCE(!irqs_disabled());
 	/*
 	 * If command already reached CMD_T_COMPLETE state within
 	 * target_complete_cmd() or CMD_T_FABRIC_STOP due to shutdown,
diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 74b646f165d4..2c3ddb3a4124 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -2954,9 +2954,6 @@ __transport_wait_for_tasks(struct se_cmd *cmd, bool fabric_stop,
 	__acquires(&cmd->t_state_lock)
 {
 
-	assert_spin_locked(&cmd->t_state_lock);
-	WARN_ON_ONCE(!irqs_disabled());
-
 	if (fabric_stop)
 		cmd->transport_state |= CMD_T_FABRIC_STOP;
 
@@ -3241,9 +3238,6 @@ static int __transport_check_aborted_status(struct se_cmd *cmd, int send_status)
 {
 	int ret;
 
-	assert_spin_locked(&cmd->t_state_lock);
-	WARN_ON_ONCE(!irqs_disabled());
-
 	if (!(cmd->transport_state & CMD_T_ABORTED))
 		return 0;
 	/*
-- 
2.16.2

[PATCH 2/2] target: remove spin_lock_assert() in __target_(attach|detach)_tg_pt_gp()

[bigeasy]

__target_attach_tg_pt_gp() and __target_detach_tg_pt_gp() check if the caller
holds lun_tg_pt_gp_lock(). Both functions are static, the callers are
acquiring the lock before the invocation of the function so the check
looks superfluous.
Remove it.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/target/target_core_alua.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/target/target_core_alua.c b/drivers/target/target_core_alua.c
index e46ca968009c..e5bda674bdbd 100644
--- a/drivers/target/target_core_alua.c
+++ b/drivers/target/target_core_alua.c
@@ -1843,8 +1843,6 @@ static void __target_attach_tg_pt_gp(struct se_lun *lun,
 {
 	struct se_dev_entry *se_deve;
 
-	assert_spin_locked(&lun->lun_tg_pt_gp_lock);
-
 	spin_lock(&tg_pt_gp->tg_pt_gp_lock);
 	lun->lun_tg_pt_gp = tg_pt_gp;
 	list_add_tail(&lun->lun_tg_pt_gp_link, &tg_pt_gp->tg_pt_gp_lun_list);
@@ -1868,8 +1866,6 @@ void target_attach_tg_pt_gp(struct se_lun *lun,
 static void __target_detach_tg_pt_gp(struct se_lun *lun,
 		struct t10_alua_tg_pt_gp *tg_pt_gp)
 {
-	assert_spin_locked(&lun->lun_tg_pt_gp_lock);
-
 	spin_lock(&tg_pt_gp->tg_pt_gp_lock);
 	list_del_init(&lun->lun_tg_pt_gp_link);
 	tg_pt_gp->tg_pt_gp_members--;
-- 
2.16.2

[PATCH 2/2] target: remove spin_lock_assert() in __target_(attach|detach)_tg_pt_gp()

[bigeasy]

__target_attach_tg_pt_gp() and __target_detach_tg_pt_gp() check if the caller
holds lun_tg_pt_gp_lock(). Both functions are static, the callers are
acquiring the lock before the invocation of the function so the check
looks superfluous.
Remove it.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/target/target_core_alua.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/target/target_core_alua.c b/drivers/target/target_core_alua.c
index e46ca968009c..e5bda674bdbd 100644
--- a/drivers/target/target_core_alua.c
+++ b/drivers/target/target_core_alua.c
@@ -1843,8 +1843,6 @@ static void __target_attach_tg_pt_gp(struct se_lun *lun,
 {
 	struct se_dev_entry *se_deve;
 
-	assert_spin_locked(&lun->lun_tg_pt_gp_lock);
-
 	spin_lock(&tg_pt_gp->tg_pt_gp_lock);
 	lun->lun_tg_pt_gp = tg_pt_gp;
 	list_add_tail(&lun->lun_tg_pt_gp_link, &tg_pt_gp->tg_pt_gp_lun_list);
@@ -1868,8 +1866,6 @@ void target_attach_tg_pt_gp(struct se_lun *lun,
 static void __target_detach_tg_pt_gp(struct se_lun *lun,
 		struct t10_alua_tg_pt_gp *tg_pt_gp)
 {
-	assert_spin_locked(&lun->lun_tg_pt_gp_lock);
-
 	spin_lock(&tg_pt_gp->tg_pt_gp_lock);
 	list_del_init(&lun->lun_tg_pt_gp_link);
 	tg_pt_gp->tg_pt_gp_members--;
-- 
2.16.2

[PATCH 1/2 v2] target: drop spin_lock_assert() + irqs_disabled() combo checks

[Sebastian Andrzej Siewior]

There are a few functions which check for if the lock is held
(spin_lock_assert()) and the interrupts are disabled (irqs_disabled()).
>From looking at the code, each function is static, the caller is near by
and does spin_lock_irq|safe(). As Linus puts it:

|It's not like this is some function that is exported to random users,
|and we should check that the calling convention is right.
|
|This looks like "it may have been useful during coding to document
|things, but it's not useful long-term".

Remove those checks.

Reported-by: Arnaldo Carvalho de Melo <acme@kernel.org>
Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
v1…2: Add credits.

 drivers/target/target_core_tmr.c       | 2 --
 drivers/target/target_core_transport.c | 6 ------
 2 files changed, 8 deletions(-)

diff --git a/drivers/target/target_core_tmr.c b/drivers/target/target_core_tmr.c
index 9c7bc1ca341a..3d35dad1de2c 100644
--- a/drivers/target/target_core_tmr.c
+++ b/drivers/target/target_core_tmr.c
@@ -114,8 +114,6 @@ static bool __target_check_io_state(struct se_cmd *se_cmd,
 {
 	struct se_session *sess = se_cmd->se_sess;
 
-	assert_spin_locked(&sess->sess_cmd_lock);
-	WARN_ON_ONCE(!irqs_disabled());
 	/*
 	 * If command already reached CMD_T_COMPLETE state within
 	 * target_complete_cmd() or CMD_T_FABRIC_STOP due to shutdown,
diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 74b646f165d4..2c3ddb3a4124 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -2954,9 +2954,6 @@ __transport_wait_for_tasks(struct se_cmd *cmd, bool fabric_stop,
 	__acquires(&cmd->t_state_lock)
 {
 
-	assert_spin_locked(&cmd->t_state_lock);
-	WARN_ON_ONCE(!irqs_disabled());
-
 	if (fabric_stop)
 		cmd->transport_state |= CMD_T_FABRIC_STOP;
 
@@ -3241,9 +3238,6 @@ static int __transport_check_aborted_status(struct se_cmd *cmd, int send_status)
 {
 	int ret;
 
-	assert_spin_locked(&cmd->t_state_lock);
-	WARN_ON_ONCE(!irqs_disabled());
-
 	if (!(cmd->transport_state & CMD_T_ABORTED))
 		return 0;
 	/*
-- 
2.16.2

[PATCH 1/2 v2] target: drop spin_lock_assert() + irqs_disabled() combo checks

[Sebastian Andrzej Siewior]

There are a few functions which check for if the lock is held
(spin_lock_assert()) and the interrupts are disabled (irqs_disabled()).
From looking at the code, each function is static, the caller is near by
and does spin_lock_irq|safe(). As Linus puts it:

|It's not like this is some function that is exported to random users,
|and we should check that the calling convention is right.
|
|This looks like "it may have been useful during coding to document
|things, but it's not useful long-term".

Remove those checks.

Reported-by: Arnaldo Carvalho de Melo <acme@kernel.org>
Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
v1…2: Add credits.

 drivers/target/target_core_tmr.c       | 2 --
 drivers/target/target_core_transport.c | 6 ------
 2 files changed, 8 deletions(-)

diff --git a/drivers/target/target_core_tmr.c b/drivers/target/target_core_tmr.c
index 9c7bc1ca341a..3d35dad1de2c 100644
--- a/drivers/target/target_core_tmr.c
+++ b/drivers/target/target_core_tmr.c
@@ -114,8 +114,6 @@ static bool __target_check_io_state(struct se_cmd *se_cmd,
 {
 	struct se_session *sess = se_cmd->se_sess;
 
-	assert_spin_locked(&sess->sess_cmd_lock);
-	WARN_ON_ONCE(!irqs_disabled());
 	/*
 	 * If command already reached CMD_T_COMPLETE state within
 	 * target_complete_cmd() or CMD_T_FABRIC_STOP due to shutdown,
diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 74b646f165d4..2c3ddb3a4124 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -2954,9 +2954,6 @@ __transport_wait_for_tasks(struct se_cmd *cmd, bool fabric_stop,
 	__acquires(&cmd->t_state_lock)
 {
 
-	assert_spin_locked(&cmd->t_state_lock);
-	WARN_ON_ONCE(!irqs_disabled());
-
 	if (fabric_stop)
 		cmd->transport_state |= CMD_T_FABRIC_STOP;
 
@@ -3241,9 +3238,6 @@ static int __transport_check_aborted_status(struct se_cmd *cmd, int send_status)
 {
 	int ret;
 
-	assert_spin_locked(&cmd->t_state_lock);
-	WARN_ON_ONCE(!irqs_disabled());
-
 	if (!(cmd->transport_state & CMD_T_ABORTED))
 		return 0;
 	/*
-- 
2.16.2

Re: [PATCH 2/2] target: remove spin_lock_assert() in __target_(attach|detach)_tg_pt_gp()

[Bart Van Assche]

On Fri, 2018-03-23 at 18:19 +0100, bigeasy@linutronix.de wrote:
> __target_attach_tg_pt_gp() and __target_detach_tg_pt_gp() check if the caller
> holds lun_tg_pt_gp_lock(). Both functions are static, the callers are
> acquiring the lock before the invocation of the function so the check
> looks superfluous.
> Remove it.

Does this check cause trouble to anyone or to a specific kernel configuration?
In other words, do we really need to remove these checks? I think that these
checks are useful as documentation to people who read the SCSI target code.
The target code is already hard to follow so I think any documentation,
especially documentation in the form of code that is checked at runtime, is
welcome.

Thanks,

Bart.

Re: [PATCH 2/2] target: remove spin_lock_assert() in __target_(attach|detach)_tg_pt_gp()

[Bart Van Assche]

T24gRnJpLCAyMDE4LTAzLTIzIGF0IDE4OjE5ICswMTAwLCBiaWdlYXN5QGxpbnV0cm9uaXguZGUg
d3JvdGU6DQo+IF9fdGFyZ2V0X2F0dGFjaF90Z19wdF9ncCgpIGFuZCBfX3RhcmdldF9kZXRhY2hf
dGdfcHRfZ3AoKSBjaGVjayBpZiB0aGUgY2FsbGVyDQo+IGhvbGRzIGx1bl90Z19wdF9ncF9sb2Nr
KCkuIEJvdGggZnVuY3Rpb25zIGFyZSBzdGF0aWMsIHRoZSBjYWxsZXJzIGFyZQ0KPiBhY3F1aXJp
bmcgdGhlIGxvY2sgYmVmb3JlIHRoZSBpbnZvY2F0aW9uIG9mIHRoZSBmdW5jdGlvbiBzbyB0aGUg
Y2hlY2sNCj4gbG9va3Mgc3VwZXJmbHVvdXMuDQo+IFJlbW92ZSBpdC4NCg0KRG9lcyB0aGlzIGNo
ZWNrIGNhdXNlIHRyb3VibGUgdG8gYW55b25lIG9yIHRvIGEgc3BlY2lmaWMga2VybmVsIGNvbmZp
Z3VyYXRpb24/DQpJbiBvdGhlciB3b3JkcywgZG8gd2UgcmVhbGx5IG5lZWQgdG8gcmVtb3ZlIHRo
ZXNlIGNoZWNrcz8gSSB0aGluayB0aGF0IHRoZXNlDQpjaGVja3MgYXJlIHVzZWZ1bCBhcyBkb2N1
bWVudGF0aW9uIHRvIHBlb3BsZSB3aG8gcmVhZCB0aGUgU0NTSSB0YXJnZXQgY29kZS4NClRoZSB0
YXJnZXQgY29kZSBpcyBhbHJlYWR5IGhhcmQgdG8gZm9sbG93IHNvIEkgdGhpbmsgYW55IGRvY3Vt
ZW50YXRpb24sDQplc3BlY2lhbGx5IGRvY3VtZW50YXRpb24gaW4gdGhlIGZvcm0gb2YgY29kZSB0
aGF0IGlzIGNoZWNrZWQgYXQgcnVudGltZSwgaXMNCndlbGNvbWUuDQoNClRoYW5rcywNCg0KQmFy
dC4NCg0KDQoNCg=

Re: [PATCH 1/2 v2] target: drop spin_lock_assert() + irqs_disabled() combo checks

[Bart Van Assche]

On Fri, 2018-03-23 at 18:36 +0100, Sebastian Andrzej Siewior wrote:
> There are a few functions which check for if the lock is held
> (spin_lock_assert()) and the interrupts are disabled (irqs_disabled()).
> > From looking at the code, each function is static, the caller is near by
> 
> and does spin_lock_irq|safe(). As Linus puts it:
> 
> > It's not like this is some function that is exported to random users,
> > and we should check that the calling convention is right.
> > 
> > This looks like "it may have been useful during coding to document
> > things, but it's not useful long-term".
> 
> Remove those checks.

Reviewed-by: Bart Van Assche <bart.vanassche@wdc.com>

Re: [PATCH 1/2 v2] target: drop spin_lock_assert() + irqs_disabled() combo checks

[Bart Van Assche]

T24gRnJpLCAyMDE4LTAzLTIzIGF0IDE4OjM2ICswMTAwLCBTZWJhc3RpYW4gQW5kcnplaiBTaWV3
aW9yIHdyb3RlOg0KPiBUaGVyZSBhcmUgYSBmZXcgZnVuY3Rpb25zIHdoaWNoIGNoZWNrIGZvciBp
ZiB0aGUgbG9jayBpcyBoZWxkDQo+IChzcGluX2xvY2tfYXNzZXJ0KCkpIGFuZCB0aGUgaW50ZXJy
dXB0cyBhcmUgZGlzYWJsZWQgKGlycXNfZGlzYWJsZWQoKSkuDQo+ID4gRnJvbSBsb29raW5nIGF0
IHRoZSBjb2RlLCBlYWNoIGZ1bmN0aW9uIGlzIHN0YXRpYywgdGhlIGNhbGxlciBpcyBuZWFyIGJ5
DQo+IA0KPiBhbmQgZG9lcyBzcGluX2xvY2tfaXJxfHNhZmUoKS4gQXMgTGludXMgcHV0cyBpdDoN
Cj4gDQo+ID4gSXQncyBub3QgbGlrZSB0aGlzIGlzIHNvbWUgZnVuY3Rpb24gdGhhdCBpcyBleHBv
cnRlZCB0byByYW5kb20gdXNlcnMsDQo+ID4gYW5kIHdlIHNob3VsZCBjaGVjayB0aGF0IHRoZSBj
YWxsaW5nIGNvbnZlbnRpb24gaXMgcmlnaHQuDQo+ID4gDQo+ID4gVGhpcyBsb29rcyBsaWtlICJp
dCBtYXkgaGF2ZSBiZWVuIHVzZWZ1bCBkdXJpbmcgY29kaW5nIHRvIGRvY3VtZW50DQo+ID4gdGhp
bmdzLCBidXQgaXQncyBub3QgdXNlZnVsIGxvbmctdGVybSIuDQo+IA0KPiBSZW1vdmUgdGhvc2Ug
Y2hlY2tzLg0KDQpSZXZpZXdlZC1ieTogQmFydCBWYW4gQXNzY2hlIDxiYXJ0LnZhbmFzc2NoZUB3
ZGMuY29tPg0KDQoNCg0K

Re: [PATCH 2/2] target: remove spin_lock_assert() in __target_(attach|detach)_tg_pt_gp()

[bigeasy]

On 2018-03-23 17:44:46 [+0000], Bart Van Assche wrote:
> On Fri, 2018-03-23 at 18:19 +0100, bigeasy@linutronix.de wrote:
> > __target_attach_tg_pt_gp() and __target_detach_tg_pt_gp() check if the caller
> > holds lun_tg_pt_gp_lock(). Both functions are static, the callers are
> > acquiring the lock before the invocation of the function so the check
> > looks superfluous.
> > Remove it.
> 
> Does this check cause trouble to anyone or to a specific kernel configuration?
Those two do not.

> In other words, do we really need to remove these checks? I think that these
> checks are useful as documentation to people who read the SCSI target code.
> The target code is already hard to follow so I think any documentation,
> especially documentation in the form of code that is checked at runtime, is
> welcome.

so if I remove those two and add a kernel doc comment instead, saying
that the caller needs to ensure that "lun->lun_tg_pt_gp_lock" is held
then we would remove the obvious runtime check and add a piece of
documentation. Would that work?

> Thanks,
> 
> Bart.

Sebastian

Re: [PATCH 2/2] target: remove spin_lock_assert() in __target_(attach|detach)_tg_pt_gp()

[bigeasy]

On 2018-03-23 17:44:46 [+0000], Bart Van Assche wrote:
> On Fri, 2018-03-23 at 18:19 +0100, bigeasy@linutronix.de wrote:
> > __target_attach_tg_pt_gp() and __target_detach_tg_pt_gp() check if the caller
> > holds lun_tg_pt_gp_lock(). Both functions are static, the callers are
> > acquiring the lock before the invocation of the function so the check
> > looks superfluous.
> > Remove it.
> 
> Does this check cause trouble to anyone or to a specific kernel configuration?
Those two do not.

> In other words, do we really need to remove these checks? I think that these
> checks are useful as documentation to people who read the SCSI target code.
> The target code is already hard to follow so I think any documentation,
> especially documentation in the form of code that is checked at runtime, is
> welcome.

so if I remove those two and add a kernel doc comment instead, saying
that the caller needs to ensure that "lun->lun_tg_pt_gp_lock" is held
then we would remove the obvious runtime check and add a piece of
documentation. Would that work?

> Thanks,
> 
> Bart.

Sebastian

Re: [PATCH 2/2] target: remove spin_lock_assert() in __target_(attach|detach)_tg_pt_gp()

[Bart Van Assche]

On Fri, 2018-03-23 at 18:50 +0100, bigeasy@linutronix.de wrote:
> On 2018-03-23 17:44:46 [+0000], Bart Van Assche wrote:
> > In other words, do we really need to remove these checks? I think that these
> > checks are useful as documentation to people who read the SCSI target code.
> > The target code is already hard to follow so I think any documentation,
> > especially documentation in the form of code that is checked at runtime, is
> > welcome.
> 
> so if I remove those two and add a kernel doc comment instead, saying
> that the caller needs to ensure that "lun->lun_tg_pt_gp_lock" is held
> then we would remove the obvious runtime check and add a piece of
> documentation. Would that work?

Comments are not verified at runtime and hence can become outdated if the code
is modified. assert_spin_locked() and lockdep_assert_held() assertions however
are verified at runtime with the proper kernel configuration options enabled.
Hence my preference for assert_spin_locked()/lockdep_assert_held() over source
code comments.

Thanks,

Bart.

Re: [PATCH 2/2] target: remove spin_lock_assert() in __target_(attach|detach)_tg_pt_gp()

[Bart Van Assche]

T24gRnJpLCAyMDE4LTAzLTIzIGF0IDE4OjUwICswMTAwLCBiaWdlYXN5QGxpbnV0cm9uaXguZGUg
d3JvdGU6DQo+IE9uIDIwMTgtMDMtMjMgMTc6NDQ6NDYgWyswMDAwXSwgQmFydCBWYW4gQXNzY2hl
IHdyb3RlOg0KPiA+IEluIG90aGVyIHdvcmRzLCBkbyB3ZSByZWFsbHkgbmVlZCB0byByZW1vdmUg
dGhlc2UgY2hlY2tzPyBJIHRoaW5rIHRoYXQgdGhlc2UNCj4gPiBjaGVja3MgYXJlIHVzZWZ1bCBh
cyBkb2N1bWVudGF0aW9uIHRvIHBlb3BsZSB3aG8gcmVhZCB0aGUgU0NTSSB0YXJnZXQgY29kZS4N
Cj4gPiBUaGUgdGFyZ2V0IGNvZGUgaXMgYWxyZWFkeSBoYXJkIHRvIGZvbGxvdyBzbyBJIHRoaW5r
IGFueSBkb2N1bWVudGF0aW9uLA0KPiA+IGVzcGVjaWFsbHkgZG9jdW1lbnRhdGlvbiBpbiB0aGUg
Zm9ybSBvZiBjb2RlIHRoYXQgaXMgY2hlY2tlZCBhdCBydW50aW1lLCBpcw0KPiA+IHdlbGNvbWUu
DQo+IA0KPiBzbyBpZiBJIHJlbW92ZSB0aG9zZSB0d28gYW5kIGFkZCBhIGtlcm5lbCBkb2MgY29t
bWVudCBpbnN0ZWFkLCBzYXlpbmcNCj4gdGhhdCB0aGUgY2FsbGVyIG5lZWRzIHRvIGVuc3VyZSB0
aGF0ICJsdW4tPmx1bl90Z19wdF9ncF9sb2NrIiBpcyBoZWxkDQo+IHRoZW4gd2Ugd291bGQgcmVt
b3ZlIHRoZSBvYnZpb3VzIHJ1bnRpbWUgY2hlY2sgYW5kIGFkZCBhIHBpZWNlIG9mDQo+IGRvY3Vt
ZW50YXRpb24uIFdvdWxkIHRoYXQgd29yaz8NCg0KQ29tbWVudHMgYXJlIG5vdCB2ZXJpZmllZCBh
dCBydW50aW1lIGFuZCBoZW5jZSBjYW4gYmVjb21lIG91dGRhdGVkIGlmIHRoZSBjb2RlDQppcyBt
b2RpZmllZC4gYXNzZXJ0X3NwaW5fbG9ja2VkKCkgYW5kIGxvY2tkZXBfYXNzZXJ0X2hlbGQoKSBh
c3NlcnRpb25zIGhvd2V2ZXINCmFyZSB2ZXJpZmllZCBhdCBydW50aW1lIHdpdGggdGhlIHByb3Bl
ciBrZXJuZWwgY29uZmlndXJhdGlvbiBvcHRpb25zIGVuYWJsZWQuDQpIZW5jZSBteSBwcmVmZXJl
bmNlIGZvciBhc3NlcnRfc3Bpbl9sb2NrZWQoKS9sb2NrZGVwX2Fzc2VydF9oZWxkKCkgb3ZlciBz
b3VyY2UNCmNvZGUgY29tbWVudHMuDQoNClRoYW5rcywNCg0KQmFydC4NCg0KDQo

Re: [PATCH] target: Use WARNON_NON_RT(!irqs_disabled())

[Arnaldo Carvalho de Melo]

Em Fri, Mar 23, 2018 at 04:55:13PM +0100, Sebastian Andrzej Siewior escreveu:
> Arnaldo, please do "[PATCH RT]" while sending patches. Then the bots
> don't complain if it applies but does not compile on !RT kernel (or so
> I've been told).

Ok, I'll try to remember that for future patches.

- Arnaldo

Re: [PATCH] target: Use WARNON_NON_RT(!irqs_disabled())

[Arnaldo Carvalho de Melo]

Em Fri, Mar 23, 2018 at 04:55:13PM +0100, Sebastian Andrzej Siewior escreveu:
> Arnaldo, please do "[PATCH RT]" while sending patches. Then the bots
> don't complain if it applies but does not compile on !RT kernel (or so
> I've been told).

Ok, I'll try to remember that for future patches.

- Arnaldo

Re: [PATCH 1/2] target: drop spin_lock_assert() + irqs_disabled() combo checks

[Steven Rostedt]

On Fri, 23 Mar 2018 18:17:36 +0100
"bigeasy@linutronix.de" <bigeasy@linutronix.de> wrote:

> There are a few functions which check for if the lock is held
> (spin_lock_assert()) and the interrupts are disabled (irqs_disabled()).
> >From looking at the code, each function is static, the caller is near by  
> and does spin_lock_irq|safe(). As Linus puts it:
> 
> |It's not like this is some function that is exported to random users,
> |and we should check that the calling convention is right.
> |
> |This looks like "it may have been useful during coding to document
> |things, but it's not useful long-term".
> 
> Remove those checks.
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>  drivers/target/target_core_tmr.c       | 2 --
>  drivers/target/target_core_transport.c | 6 ------
>  2 files changed, 8 deletions(-)
> 
> diff --git a/drivers/target/target_core_tmr.c b/drivers/target/target_core_tmr.c
> index 9c7bc1ca341a..3d35dad1de2c 100644
> --- a/drivers/target/target_core_tmr.c
> +++ b/drivers/target/target_core_tmr.c

Can you add a comment above the functions though?

/* Expects to have se_cmd->se_sess->sess_cmd_lock held */

> @@ -114,8 +114,6 @@ static bool __target_check_io_state(struct se_cmd *se_cmd,
>  {
>  	struct se_session *sess = se_cmd->se_sess;
>  
> -	assert_spin_locked(&sess->sess_cmd_lock);
> -	WARN_ON_ONCE(!irqs_disabled());
>  	/*
>  	 * If command already reached CMD_T_COMPLETE state within
>  	 * target_complete_cmd() or CMD_T_FABRIC_STOP due to shutdown,
> diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
> index 74b646f165d4..2c3ddb3a4124 100644
> --- a/drivers/target/target_core_transport.c
> +++ b/drivers/target/target_core_transport.c

/* Expects to have cmd->t_state_lock held */

> @@ -2954,9 +2954,6 @@ __transport_wait_for_tasks(struct se_cmd *cmd, bool fabric_stop,
>  	__acquires(&cmd->t_state_lock)
>  {
>  
> -	assert_spin_locked(&cmd->t_state_lock);
> -	WARN_ON_ONCE(!irqs_disabled());
> -
>  	if (fabric_stop)
>  		cmd->transport_state |= CMD_T_FABRIC_STOP;
>  

/* Expects to have cmd->t_state_lock held */

> @@ -3241,9 +3238,6 @@ static int __transport_check_aborted_status(struct se_cmd *cmd, int send_status)
>  {
>  	int ret;
>  
> -	assert_spin_locked(&cmd->t_state_lock);
> -	WARN_ON_ONCE(!irqs_disabled());
> -
>  	if (!(cmd->transport_state & CMD_T_ABORTED))
>  		return 0;
>  	/*

-- Steve

Re: [PATCH 1/2] target: drop spin_lock_assert() + irqs_disabled() combo checks

[Steven Rostedt]

On Fri, 23 Mar 2018 18:17:36 +0100
"bigeasy@linutronix.de" <bigeasy@linutronix.de> wrote:

> There are a few functions which check for if the lock is held
> (spin_lock_assert()) and the interrupts are disabled (irqs_disabled()).
> >From looking at the code, each function is static, the caller is near by  
> and does spin_lock_irq|safe(). As Linus puts it:
> 
> |It's not like this is some function that is exported to random users,
> |and we should check that the calling convention is right.
> |
> |This looks like "it may have been useful during coding to document
> |things, but it's not useful long-term".
> 
> Remove those checks.
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>  drivers/target/target_core_tmr.c       | 2 --
>  drivers/target/target_core_transport.c | 6 ------
>  2 files changed, 8 deletions(-)
> 
> diff --git a/drivers/target/target_core_tmr.c b/drivers/target/target_core_tmr.c
> index 9c7bc1ca341a..3d35dad1de2c 100644
> --- a/drivers/target/target_core_tmr.c
> +++ b/drivers/target/target_core_tmr.c

Can you add a comment above the functions though?

/* Expects to have se_cmd->se_sess->sess_cmd_lock held */

> @@ -114,8 +114,6 @@ static bool __target_check_io_state(struct se_cmd *se_cmd,
>  {
>  	struct se_session *sess = se_cmd->se_sess;
>  
> -	assert_spin_locked(&sess->sess_cmd_lock);
> -	WARN_ON_ONCE(!irqs_disabled());
>  	/*
>  	 * If command already reached CMD_T_COMPLETE state within
>  	 * target_complete_cmd() or CMD_T_FABRIC_STOP due to shutdown,
> diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
> index 74b646f165d4..2c3ddb3a4124 100644
> --- a/drivers/target/target_core_transport.c
> +++ b/drivers/target/target_core_transport.c

/* Expects to have cmd->t_state_lock held */

> @@ -2954,9 +2954,6 @@ __transport_wait_for_tasks(struct se_cmd *cmd, bool fabric_stop,
>  	__acquires(&cmd->t_state_lock)
>  {
>  
> -	assert_spin_locked(&cmd->t_state_lock);
> -	WARN_ON_ONCE(!irqs_disabled());
> -
>  	if (fabric_stop)
>  		cmd->transport_state |= CMD_T_FABRIC_STOP;
>  

/* Expects to have cmd->t_state_lock held */

> @@ -3241,9 +3238,6 @@ static int __transport_check_aborted_status(struct se_cmd *cmd, int send_status)
>  {
>  	int ret;
>  
> -	assert_spin_locked(&cmd->t_state_lock);
> -	WARN_ON_ONCE(!irqs_disabled());
> -
>  	if (!(cmd->transport_state & CMD_T_ABORTED))
>  		return 0;
>  	/*

-- Steve

Re: [PATCH 2/2] target: remove spin_lock_assert() in __target_(attach|detach)_tg_pt_gp()

[Steven Rostedt]

On Fri, 23 Mar 2018 17:55:54 +0000
Bart Van Assche <Bart.VanAssche@wdc.com> wrote:

> Comments are not verified at runtime and hence can become outdated if the code
> is modified. assert_spin_locked() and lockdep_assert_held() assertions however
> are verified at runtime with the proper kernel configuration options enabled.
> Hence my preference for assert_spin_locked()/lockdep_assert_held() over source
> code comments.

Asserts are fine, but when the code is static and used close to the
caller, asserts are overkill. A comment at the beginning of a function
should suffice. We do that all over the kernel for functions like that.

The asserts are there when the code can be called from other files, or
in multiple places.

-- Steve

Re: [PATCH 2/2] target: remove spin_lock_assert() in __target_(attach|detach)_tg_pt_gp()

[Steven Rostedt]

On Fri, 23 Mar 2018 17:55:54 +0000
Bart Van Assche <Bart.VanAssche@wdc.com> wrote:

> Comments are not verified at runtime and hence can become outdated if the code
> is modified. assert_spin_locked() and lockdep_assert_held() assertions however
> are verified at runtime with the proper kernel configuration options enabled.
> Hence my preference for assert_spin_locked()/lockdep_assert_held() over source
> code comments.

Asserts are fine, but when the code is static and used close to the
caller, asserts are overkill. A comment at the beginning of a function
should suffice. We do that all over the kernel for functions like that.

The asserts are there when the code can be called from other files, or
in multiple places.

-- Steve

Re: [PATCH 1/2] target: drop spin_lock_assert() + irqs_disabled() combo checks

[bigeasy]

On 2018-03-26 11:13:59 [-0400], Steven Rostedt wrote:
> > diff --git a/drivers/target/target_core_tmr.c b/drivers/target/target_core_tmr.c
> > index 9c7bc1ca341a..3d35dad1de2c 100644
> > --- a/drivers/target/target_core_tmr.c
> > +++ b/drivers/target/target_core_tmr.c
> 
> Can you add a comment above the functions though?
> 
> /* Expects to have se_cmd->se_sess->sess_cmd_lock held */

I could. I haven't heard from Bart / Nicholas about their opinion. I
know, we do this other places but Bart was against this kind of
annotation in 2/2 of this thread.
So I am waiting to hear from them :)

> 
> -- Steve

Sebastian

Re: [PATCH 1/2] target: drop spin_lock_assert() + irqs_disabled() combo checks

[bigeasy]

On 2018-03-26 11:13:59 [-0400], Steven Rostedt wrote:
> > diff --git a/drivers/target/target_core_tmr.c b/drivers/target/target_core_tmr.c
> > index 9c7bc1ca341a..3d35dad1de2c 100644
> > --- a/drivers/target/target_core_tmr.c
> > +++ b/drivers/target/target_core_tmr.c
> 
> Can you add a comment above the functions though?
> 
> /* Expects to have se_cmd->se_sess->sess_cmd_lock held */

I could. I haven't heard from Bart / Nicholas about their opinion. I
know, we do this other places but Bart was against this kind of
annotation in 2/2 of this thread.
So I am waiting to hear from them :)

> 
> -- Steve

Sebastian

Re: [PATCH 1/2] target: drop spin_lock_assert() + irqs_disabled() combo checks

[Bart Van Assche]

On Wed, 2018-03-28 at 12:15 +0200, bigeasy@linutronix.de wrote:
> On 2018-03-26 11:13:59 [-0400], Steven Rostedt wrote:
> > > diff --git a/drivers/target/target_core_tmr.c b/drivers/target/target_core_tmr.c
> > > index 9c7bc1ca341a..3d35dad1de2c 100644
> > > --- a/drivers/target/target_core_tmr.c
> > > +++ b/drivers/target/target_core_tmr.c
> > 
> > Can you add a comment above the functions though?
> > 
> > /* Expects to have se_cmd->se_sess->sess_cmd_lock held */
> 
> I could. I haven't heard from Bart / Nicholas about their opinion. I
> know, we do this other places but Bart was against this kind of
> annotation in 2/2 of this thread.
> So I am waiting to hear from them :)

The names of the two functions touched by patch 1/2 start with a double
underscore. That by itself is already a hint that these should be called with
a lock held (I know that this is not a universal convention in the Linux
kernel). I'm fine either way - either with patch 1/2 as posted or patch 1/2
with the above comment added.

Bart.

Re: [PATCH 1/2] target: drop spin_lock_assert() + irqs_disabled() combo checks

[Bart Van Assche]

T24gV2VkLCAyMDE4LTAzLTI4IGF0IDEyOjE1ICswMjAwLCBiaWdlYXN5QGxpbnV0cm9uaXguZGUg
d3JvdGU6DQo+IE9uIDIwMTgtMDMtMjYgMTE6MTM6NTkgWy0wNDAwXSwgU3RldmVuIFJvc3RlZHQg
d3JvdGU6DQo+ID4gPiBkaWZmIC0tZ2l0IGEvZHJpdmVycy90YXJnZXQvdGFyZ2V0X2NvcmVfdG1y
LmMgYi9kcml2ZXJzL3RhcmdldC90YXJnZXRfY29yZV90bXIuYw0KPiA+ID4gaW5kZXggOWM3YmMx
Y2EzNDFhLi4zZDM1ZGFkMWRlMmMgMTAwNjQ0DQo+ID4gPiAtLS0gYS9kcml2ZXJzL3RhcmdldC90
YXJnZXRfY29yZV90bXIuYw0KPiA+ID4gKysrIGIvZHJpdmVycy90YXJnZXQvdGFyZ2V0X2NvcmVf
dG1yLmMNCj4gPiANCj4gPiBDYW4geW91IGFkZCBhIGNvbW1lbnQgYWJvdmUgdGhlIGZ1bmN0aW9u
cyB0aG91Z2g/DQo+ID4gDQo+ID4gLyogRXhwZWN0cyB0byBoYXZlIHNlX2NtZC0+c2Vfc2Vzcy0+
c2Vzc19jbWRfbG9jayBoZWxkICovDQo+IA0KPiBJIGNvdWxkLiBJIGhhdmVuJ3QgaGVhcmQgZnJv
bSBCYXJ0IC8gTmljaG9sYXMgYWJvdXQgdGhlaXIgb3Bpbmlvbi4gSQ0KPiBrbm93LCB3ZSBkbyB0
aGlzIG90aGVyIHBsYWNlcyBidXQgQmFydCB3YXMgYWdhaW5zdCB0aGlzIGtpbmQgb2YNCj4gYW5u
b3RhdGlvbiBpbiAyLzIgb2YgdGhpcyB0aHJlYWQuDQo+IFNvIEkgYW0gd2FpdGluZyB0byBoZWFy
IGZyb20gdGhlbSA6KQ0KDQpUaGUgbmFtZXMgb2YgdGhlIHR3byBmdW5jdGlvbnMgdG91Y2hlZCBi
eSBwYXRjaCAxLzIgc3RhcnQgd2l0aCBhIGRvdWJsZQ0KdW5kZXJzY29yZS4gVGhhdCBieSBpdHNl
bGYgaXMgYWxyZWFkeSBhIGhpbnQgdGhhdCB0aGVzZSBzaG91bGQgYmUgY2FsbGVkIHdpdGgNCmEg
bG9jayBoZWxkIChJIGtub3cgdGhhdCB0aGlzIGlzIG5vdCBhIHVuaXZlcnNhbCBjb252ZW50aW9u
IGluIHRoZSBMaW51eA0Ka2VybmVsKS4gSSdtIGZpbmUgZWl0aGVyIHdheSAtIGVpdGhlciB3aXRo
IHBhdGNoIDEvMiBhcyBwb3N0ZWQgb3IgcGF0Y2ggMS8yDQp3aXRoIHRoZSBhYm92ZSBjb21tZW50
IGFkZGVkLg0KDQpCYXJ0Lg0KDQoNCg0KDQo

Re: [PATCH 1/2] target: drop spin_lock_assert() + irqs_disabled() combo checks

[bigeasy]

On 2018-03-28 15:05:41 [+0000], Bart Van Assche wrote:
> The names of the two functions touched by patch 1/2 start with a double
> underscore. That by itself is already a hint that these should be called with
> a lock held (I know that this is not a universal convention in the Linux
> kernel). I'm fine either way - either with patch 1/2 as posted or patch 1/2
> with the above comment added.

Okay. In that case let me update 1/2.
But 2/2 with the comment as Steven suggested is still a no no for you?

> Bart.

Sebastian

Re: [PATCH 1/2] target: drop spin_lock_assert() + irqs_disabled() combo checks

[bigeasy]

On 2018-03-28 15:05:41 [+0000], Bart Van Assche wrote:
> The names of the two functions touched by patch 1/2 start with a double
> underscore. That by itself is already a hint that these should be called with
> a lock held (I know that this is not a universal convention in the Linux
> kernel). I'm fine either way - either with patch 1/2 as posted or patch 1/2
> with the above comment added.

Okay. In that case let me update 1/2.
But 2/2 with the comment as Steven suggested is still a no no for you?

> Bart.

Sebastian

Re: [PATCH 1/2] target: drop spin_lock_assert() + irqs_disabled() combo checks

[Bart Van Assche]

On 03/28/18 08:14, bigeasy@linutronix.de wrote:
> On 2018-03-28 15:05:41 [+0000], Bart Van Assche wrote:
>> The names of the two functions touched by patch 1/2 start with a double
>> underscore. That by itself is already a hint that these should be called with
>> a lock held (I know that this is not a universal convention in the Linux
>> kernel). I'm fine either way - either with patch 1/2 as posted or patch 1/2
>> with the above comment added.
> 
> Okay. In that case let me update 1/2.
> But 2/2 with the comment as Steven suggested is still a no no for you?

Although I'm not enthusiast about patch 2/2, if others agree with that 
patch I'm fine with that patch being sent upstream.

Thanks,

Bart.

Re: [PATCH 1/2] target: drop spin_lock_assert() + irqs_disabled() combo checks

[Bart Van Assche]

On 03/28/18 08:14, bigeasy@linutronix.de wrote:
> On 2018-03-28 15:05:41 [+0000], Bart Van Assche wrote:
>> The names of the two functions touched by patch 1/2 start with a double
>> underscore. That by itself is already a hint that these should be called with
>> a lock held (I know that this is not a universal convention in the Linux
>> kernel). I'm fine either way - either with patch 1/2 as posted or patch 1/2
>> with the above comment added.
> 
> Okay. In that case let me update 1/2.
> But 2/2 with the comment as Steven suggested is still a no no for you?

Although I'm not enthusiast about patch 2/2, if others agree with that 
patch I'm fine with that patch being sent upstream.

Thanks,

Bart.

Re: [PATCH 1/2] target: drop spin_lock_assert() + irqs_disabled() combo checks

[bigeasy]

On 2018-03-28 08:31:38 [-0700], Bart Van Assche wrote:
> On 03/28/18 08:14, bigeasy@linutronix.de wrote:
> > On 2018-03-28 15:05:41 [+0000], Bart Van Assche wrote:
> > > The names of the two functions touched by patch 1/2 start with a double
> > > underscore. That by itself is already a hint that these should be called with
> > > a lock held (I know that this is not a universal convention in the Linux
> > > kernel). I'm fine either way - either with patch 1/2 as posted or patch 1/2
> > > with the above comment added.
> > 
> > Okay. In that case let me update 1/2.
> > But 2/2 with the comment as Steven suggested is still a no no for you?
> 
> Although I'm not enthusiast about patch 2/2, if others agree with that patch
> I'm fine with that patch being sent upstream.

I've been waiting for something to happen but nobody replied.
Bart, you were fine with 1/2 as posted but not too happy about 2/2.
Assuming we keep 1/2 as is and I remove just the
"WARN_ON_ONCE(!irqs_disabled());" from 2/2 (keeping the
assert_spin_locked()), would that improve your mood? Lockdep would still
perform full validation, yell if __transport_check_aborted_status() was
invoked without locking and also yell abut missing IRQ-save at locking
time of ->t_state_lock).

> Thanks,
> 
> Bart.

Sebastian

Re: [PATCH 1/2] target: drop spin_lock_assert() + irqs_disabled() combo checks

[bigeasy]

On 2018-03-28 08:31:38 [-0700], Bart Van Assche wrote:
> On 03/28/18 08:14, bigeasy@linutronix.de wrote:
> > On 2018-03-28 15:05:41 [+0000], Bart Van Assche wrote:
> > > The names of the two functions touched by patch 1/2 start with a double
> > > underscore. That by itself is already a hint that these should be called with
> > > a lock held (I know that this is not a universal convention in the Linux
> > > kernel). I'm fine either way - either with patch 1/2 as posted or patch 1/2
> > > with the above comment added.
> > 
> > Okay. In that case let me update 1/2.
> > But 2/2 with the comment as Steven suggested is still a no no for you?
> 
> Although I'm not enthusiast about patch 2/2, if others agree with that patch
> I'm fine with that patch being sent upstream.

I've been waiting for something to happen but nobody replied.
Bart, you were fine with 1/2 as posted but not too happy about 2/2.
Assuming we keep 1/2 as is and I remove just the
"WARN_ON_ONCE(!irqs_disabled());" from 2/2 (keeping the
assert_spin_locked()), would that improve your mood? Lockdep would still
perform full validation, yell if __transport_check_aborted_status() was
invoked without locking and also yell abut missing IRQ-save at locking
time of ->t_state_lock).

> Thanks,
> 
> Bart.

Sebastian

Re: [PATCH 1/2] target: drop spin_lock_assert() + irqs_disabled() combo checks

[Bart Van Assche]

On Mon, 2018-05-28 at 16:35 +0200, bigeasy@linutronix.de wrote:
> On 2018-03-28 08:31:38 [-0700], Bart Van Assche wrote:
> > On 03/28/18 08:14, bigeasy@linutronix.de wrote:
> > > On 2018-03-28 15:05:41 [+0000], Bart Van Assche wrote:
> > > > The names of the two functions touched by patch 1/2 start with a double
> > > > underscore. That by itself is already a hint that these should be called with
> > > > a lock held (I know that this is not a universal convention in the Linux
> > > > kernel). I'm fine either way - either with patch 1/2 as posted or patch 1/2
> > > > with the above comment added.
> > > 
> > > Okay. In that case let me update 1/2.
> > > But 2/2 with the comment as Steven suggested is still a no no for you?
> > 
> > Although I'm not enthusiast about patch 2/2, if others agree with that patch
> > I'm fine with that patch being sent upstream.
> 
> I've been waiting for something to happen but nobody replied.
> Bart, you were fine with 1/2 as posted but not too happy about 2/2.
> Assuming we keep 1/2 as is and I remove just the
> "WARN_ON_ONCE(!irqs_disabled());" from 2/2 (keeping the
> assert_spin_locked()), would that improve your mood? Lockdep would still
> perform full validation, yell if __transport_check_aborted_status() was
> invoked without locking and also yell abut missing IRQ-save at locking
> time of ->t_state_lock).

Would adding WARN_ON_ONCE(!irqs_disabled()) work fine with an RT kernel?

Thanks,

Bart.

Re: [PATCH 1/2] target: drop spin_lock_assert() + irqs_disabled() combo checks

[Bart Van Assche]

T24gTW9uLCAyMDE4LTA1LTI4IGF0IDE2OjM1ICswMjAwLCBiaWdlYXN5QGxpbnV0cm9uaXguZGUg
d3JvdGU6DQo+IE9uIDIwMTgtMDMtMjggMDg6MzE6MzggWy0wNzAwXSwgQmFydCBWYW4gQXNzY2hl
IHdyb3RlOg0KPiA+IE9uIDAzLzI4LzE4IDA4OjE0LCBiaWdlYXN5QGxpbnV0cm9uaXguZGUgd3Jv
dGU6DQo+ID4gPiBPbiAyMDE4LTAzLTI4IDE1OjA1OjQxIFsrMDAwMF0sIEJhcnQgVmFuIEFzc2No
ZSB3cm90ZToNCj4gPiA+ID4gVGhlIG5hbWVzIG9mIHRoZSB0d28gZnVuY3Rpb25zIHRvdWNoZWQg
YnkgcGF0Y2ggMS8yIHN0YXJ0IHdpdGggYSBkb3VibGUNCj4gPiA+ID4gdW5kZXJzY29yZS4gVGhh
dCBieSBpdHNlbGYgaXMgYWxyZWFkeSBhIGhpbnQgdGhhdCB0aGVzZSBzaG91bGQgYmUgY2FsbGVk
IHdpdGgNCj4gPiA+ID4gYSBsb2NrIGhlbGQgKEkga25vdyB0aGF0IHRoaXMgaXMgbm90IGEgdW5p
dmVyc2FsIGNvbnZlbnRpb24gaW4gdGhlIExpbnV4DQo+ID4gPiA+IGtlcm5lbCkuIEknbSBmaW5l
IGVpdGhlciB3YXkgLSBlaXRoZXIgd2l0aCBwYXRjaCAxLzIgYXMgcG9zdGVkIG9yIHBhdGNoIDEv
Mg0KPiA+ID4gPiB3aXRoIHRoZSBhYm92ZSBjb21tZW50IGFkZGVkLg0KPiA+ID4gDQo+ID4gPiBP
a2F5LiBJbiB0aGF0IGNhc2UgbGV0IG1lIHVwZGF0ZSAxLzIuDQo+ID4gPiBCdXQgMi8yIHdpdGgg
dGhlIGNvbW1lbnQgYXMgU3RldmVuIHN1Z2dlc3RlZCBpcyBzdGlsbCBhIG5vIG5vIGZvciB5b3U/
DQo+ID4gDQo+ID4gQWx0aG91Z2ggSSdtIG5vdCBlbnRodXNpYXN0IGFib3V0IHBhdGNoIDIvMiwg
aWYgb3RoZXJzIGFncmVlIHdpdGggdGhhdCBwYXRjaA0KPiA+IEknbSBmaW5lIHdpdGggdGhhdCBw
YXRjaCBiZWluZyBzZW50IHVwc3RyZWFtLg0KPiANCj4gSSd2ZSBiZWVuIHdhaXRpbmcgZm9yIHNv
bWV0aGluZyB0byBoYXBwZW4gYnV0IG5vYm9keSByZXBsaWVkLg0KPiBCYXJ0LCB5b3Ugd2VyZSBm
aW5lIHdpdGggMS8yIGFzIHBvc3RlZCBidXQgbm90IHRvbyBoYXBweSBhYm91dCAyLzIuDQo+IEFz
c3VtaW5nIHdlIGtlZXAgMS8yIGFzIGlzIGFuZCBJIHJlbW92ZSBqdXN0IHRoZQ0KPiAiV0FSTl9P
Tl9PTkNFKCFpcnFzX2Rpc2FibGVkKCkpOyIgZnJvbSAyLzIgKGtlZXBpbmcgdGhlDQo+IGFzc2Vy
dF9zcGluX2xvY2tlZCgpKSwgd291bGQgdGhhdCBpbXByb3ZlIHlvdXIgbW9vZD8gTG9ja2RlcCB3
b3VsZCBzdGlsbA0KPiBwZXJmb3JtIGZ1bGwgdmFsaWRhdGlvbiwgeWVsbCBpZiBfX3RyYW5zcG9y
dF9jaGVja19hYm9ydGVkX3N0YXR1cygpIHdhcw0KPiBpbnZva2VkIHdpdGhvdXQgbG9ja2luZyBh
bmQgYWxzbyB5ZWxsIGFidXQgbWlzc2luZyBJUlEtc2F2ZSBhdCBsb2NraW5nDQo+IHRpbWUgb2Yg
LT50X3N0YXRlX2xvY2spLg0KDQpXb3VsZCBhZGRpbmcgV0FSTl9PTl9PTkNFKCFpcnFzX2Rpc2Fi
bGVkKCkpIHdvcmsgZmluZSB3aXRoIGFuIFJUIGtlcm5lbD8NCg0KVGhhbmtzLA0KDQpCYXJ0Lg0K
DQoNCg0KDQo

Re: [PATCH 1/2] target: drop spin_lock_assert() + irqs_disabled() combo checks

[bigeasy]

On 2018-06-04 07:02:15 [+0000], Bart Van Assche wrote:
> 
> Would adding WARN_ON_ONCE(!irqs_disabled()) work fine with an RT kernel?

no, because the interrupts are not disabled at this point on RT.
Lockdep works fine on RT.

> Thanks,
> 
> Bart.

Sebastian

Re: [PATCH 1/2] target: drop spin_lock_assert() + irqs_disabled() combo checks

[bigeasy]

On 2018-06-04 07:02:15 [+0000], Bart Van Assche wrote:
> 
> Would adding WARN_ON_ONCE(!irqs_disabled()) work fine with an RT kernel?

no, because the interrupts are not disabled at this point on RT.
Lockdep works fine on RT.

> Thanks,
> 
> Bart.

Sebastian