* [PATCH] hw: Fix permissions for OCRDMA_RESET_STATS @ 2017-08-08 15:56 Anton Vasilyev 2017-08-15 18:31 ` Leon Romanovsky 2017-12-21 2:59 ` Jason Gunthorpe 0 siblings, 2 replies; 7+ messages in thread From: Anton Vasilyev @ 2017-08-08 15:56 UTC (permalink / raw) To: Selvin Xavier Cc: Anton Vasilyev, Devesh Sharma, Doug Ledford, Sean Hefty, Hal Rosenstock, Leon Romanovsky, Michael Mera, linux-rdma, linux-kernel, ldv-project Debugfs file reset_stats is created with S_IRUSR permissions, but ocrdma_dbgfs_ops_read() doesn't support OCRDMA_RESET_STATS, whereas ocrdma_dbgfs_ops_write() supports only OCRDMA_RESET_STATS. The patch fixes misstype with permissions. Found by Linux Driver Verification project (linuxtesting.org). Signed-off-by: Anton Vasilyev <vasilyev@ispras.ru> --- drivers/infiniband/hw/ocrdma/ocrdma_stats.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/infiniband/hw/ocrdma/ocrdma_stats.c b/drivers/infiniband/hw/ocrdma/ocrdma_stats.c index 66056f9..d42c617 100644 --- a/drivers/infiniband/hw/ocrdma/ocrdma_stats.c +++ b/drivers/infiniband/hw/ocrdma/ocrdma_stats.c @@ -834,7 +834,7 @@ void ocrdma_add_port_stats(struct ocrdma_dev *dev) dev->reset_stats.type = OCRDMA_RESET_STATS; dev->reset_stats.dev = dev; - if (!debugfs_create_file("reset_stats", S_IRUSR, dev->dir, + if (!debugfs_create_file("reset_stats", S_IWUSR, dev->dir, &dev->reset_stats, &ocrdma_dbg_ops)) goto err; -- 2.7.4 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] hw: Fix permissions for OCRDMA_RESET_STATS 2017-08-08 15:56 [PATCH] hw: Fix permissions for OCRDMA_RESET_STATS Anton Vasilyev @ 2017-08-15 18:31 ` Leon Romanovsky 2017-12-19 10:29 ` Selvin Xavier 2017-12-21 2:59 ` Jason Gunthorpe 1 sibling, 1 reply; 7+ messages in thread From: Leon Romanovsky @ 2017-08-15 18:31 UTC (permalink / raw) To: Anton Vasilyev Cc: Selvin Xavier, Devesh Sharma, Doug Ledford, Sean Hefty, Hal Rosenstock, Michael Mera, linux-rdma, linux-kernel, ldv-project [-- Attachment #1: Type: text/plain, Size: 2381 bytes --] On Tue, Aug 08, 2017 at 06:56:37PM +0300, Anton Vasilyev wrote: > Debugfs file reset_stats is created with S_IRUSR permissions, > but ocrdma_dbgfs_ops_read() doesn't support OCRDMA_RESET_STATS, > whereas ocrdma_dbgfs_ops_write() supports only OCRDMA_RESET_STATS. > > The patch fixes misstype with permissions. > > Found by Linux Driver Verification project (linuxtesting.org). > > Signed-off-by: Anton Vasilyev <vasilyev@ispras.ru> > --- > drivers/infiniband/hw/ocrdma/ocrdma_stats.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/infiniband/hw/ocrdma/ocrdma_stats.c b/drivers/infiniband/hw/ocrdma/ocrdma_stats.c > index 66056f9..d42c617 100644 > --- a/drivers/infiniband/hw/ocrdma/ocrdma_stats.c > +++ b/drivers/infiniband/hw/ocrdma/ocrdma_stats.c > @@ -834,7 +834,7 @@ void ocrdma_add_port_stats(struct ocrdma_dev *dev) > > dev->reset_stats.type = OCRDMA_RESET_STATS; > dev->reset_stats.dev = dev; > - if (!debugfs_create_file("reset_stats", S_IRUSR, dev->dir, > + if (!debugfs_create_file("reset_stats", S_IWUSR, dev->dir, > &dev->reset_stats, &ocrdma_dbg_ops)) > goto err; > If I didn't miss anything, the reset_stats knob wouldn't work. Any read of statistics from that debugfs will trigger the call to ocrdma_update_stats and it will call to ocrdma_mbx_rdma_stats(dev, false). For write operations to that "reset_stats", there is call to ocrdma_mbx_rdma_stats with second argument (reset) as true. But it doesn't do much with that "reset" argument, except update of reset_stats. git grep reset_stats drivers/infiniband/hw/ocrdma/* drivers/net/ethernet/emulex/ drivers/infiniband/hw/ocrdma/ocrdma.h: struct ocrdma_stats reset_stats; drivers/infiniband/hw/ocrdma/ocrdma_hw.c: req->reset_stats = reset; drivers/infiniband/hw/ocrdma/ocrdma_sli.h: u8 reset_stats; drivers/infiniband/hw/ocrdma/ocrdma_stats.c: dev->reset_stats.type = OCRDMA_RESET_STATS; drivers/infiniband/hw/ocrdma/ocrdma_stats.c: dev->reset_stats.dev = dev; drivers/infiniband/hw/ocrdma/ocrdma_stats.c: if (!debugfs_create_file("reset_stats", S_IRUSR, dev->dir, drivers/infiniband/hw/ocrdma/ocrdma_stats.c: &dev->reset_stats, &ocrdma_dbg_ops)) drivers/net/ethernet/emulex/benet/be_cmds.c: req->cmd_params.params.reset_stats = 0; drivers/net/ethernet/emulex/benet/be_cmds.h: u8 reset_stats; Thanks [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] hw: Fix permissions for OCRDMA_RESET_STATS 2017-08-15 18:31 ` Leon Romanovsky @ 2017-12-19 10:29 ` Selvin Xavier 2017-12-19 12:30 ` Leon Romanovsky 0 siblings, 1 reply; 7+ messages in thread From: Selvin Xavier @ 2017-12-19 10:29 UTC (permalink / raw) To: Leon Romanovsky Cc: Anton Vasilyev, Devesh Sharma, Doug Ledford, Sean Hefty, Hal Rosenstock, Michael Mera, linux-rdma, linux-kernel, ldv-project On Wed, Aug 16, 2017 at 12:01 AM, Leon Romanovsky <leon@kernel.org> wrote: > On Tue, Aug 08, 2017 at 06:56:37PM +0300, Anton Vasilyev wrote: >> Debugfs file reset_stats is created with S_IRUSR permissions, >> but ocrdma_dbgfs_ops_read() doesn't support OCRDMA_RESET_STATS, >> whereas ocrdma_dbgfs_ops_write() supports only OCRDMA_RESET_STATS. >> >> The patch fixes misstype with permissions. >> >> Found by Linux Driver Verification project (linuxtesting.org). >> >> Signed-off-by: Anton Vasilyev <vasilyev@ispras.ru> >> --- >> drivers/infiniband/hw/ocrdma/ocrdma_stats.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/infiniband/hw/ocrdma/ocrdma_stats.c b/drivers/infiniband/hw/ocrdma/ocrdma_stats.c >> index 66056f9..d42c617 100644 >> --- a/drivers/infiniband/hw/ocrdma/ocrdma_stats.c >> +++ b/drivers/infiniband/hw/ocrdma/ocrdma_stats.c >> @@ -834,7 +834,7 @@ void ocrdma_add_port_stats(struct ocrdma_dev *dev) >> >> dev->reset_stats.type = OCRDMA_RESET_STATS; >> dev->reset_stats.dev = dev; >> - if (!debugfs_create_file("reset_stats", S_IRUSR, dev->dir, >> + if (!debugfs_create_file("reset_stats", S_IWUSR, dev->dir, >> &dev->reset_stats, &ocrdma_dbg_ops)) >> goto err; >> > > If I didn't miss anything, the reset_stats knob wouldn't work. > > Any read of statistics from that debugfs will trigger the call to > ocrdma_update_stats and it will call to ocrdma_mbx_rdma_stats(dev, false). > > For write operations to that "reset_stats", there is call > to ocrdma_mbx_rdma_stats with second argument (reset) as true. > > But it doesn't do much with that "reset" argument, except update of reset_stats. > > git grep reset_stats drivers/infiniband/hw/ocrdma/* drivers/net/ethernet/emulex/ > drivers/infiniband/hw/ocrdma/ocrdma.h: struct ocrdma_stats reset_stats; > drivers/infiniband/hw/ocrdma/ocrdma_hw.c: req->reset_stats = reset; "reset" filed is passed down to the FW mailbox command to reset the stats maintained by FW. req is a pointer to dev->stats_mem.va and this is used to send down the command to FW. status = ocrdma_nonemb_mbx_cmd(dev, mqe, dev->stats_mem.va); > drivers/infiniband/hw/ocrdma/ocrdma_sli.h: u8 reset_stats; > drivers/infiniband/hw/ocrdma/ocrdma_stats.c: dev->reset_stats.type = OCRDMA_RESET_STATS; > drivers/infiniband/hw/ocrdma/ocrdma_stats.c: dev->reset_stats.dev = dev; > drivers/infiniband/hw/ocrdma/ocrdma_stats.c: if (!debugfs_create_file("reset_stats", S_IRUSR, dev->dir, > drivers/infiniband/hw/ocrdma/ocrdma_stats.c: &dev->reset_stats, &ocrdma_dbg_ops)) > drivers/net/ethernet/emulex/benet/be_cmds.c: req->cmd_params.params.reset_stats = 0; > drivers/net/ethernet/emulex/benet/be_cmds.h: u8 reset_stats; > > Thanks The patch looks good to me. Acked-by: Selvin Xavier <selvin.xavier@broadcom.com> Thanks Selvin ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] hw: Fix permissions for OCRDMA_RESET_STATS 2017-12-19 10:29 ` Selvin Xavier @ 2017-12-19 12:30 ` Leon Romanovsky 2017-12-19 17:44 ` Selvin Xavier 0 siblings, 1 reply; 7+ messages in thread From: Leon Romanovsky @ 2017-12-19 12:30 UTC (permalink / raw) To: Selvin Xavier Cc: Anton Vasilyev, Devesh Sharma, Doug Ledford, Sean Hefty, Hal Rosenstock, Michael Mera, linux-rdma, linux-kernel, ldv-project [-- Attachment #1: Type: text/plain, Size: 3914 bytes --] On Tue, Dec 19, 2017 at 03:59:30PM +0530, Selvin Xavier wrote: > On Wed, Aug 16, 2017 at 12:01 AM, Leon Romanovsky <leon@kernel.org> wrote: > > On Tue, Aug 08, 2017 at 06:56:37PM +0300, Anton Vasilyev wrote: > >> Debugfs file reset_stats is created with S_IRUSR permissions, > >> but ocrdma_dbgfs_ops_read() doesn't support OCRDMA_RESET_STATS, > >> whereas ocrdma_dbgfs_ops_write() supports only OCRDMA_RESET_STATS. > >> > >> The patch fixes misstype with permissions. > >> > >> Found by Linux Driver Verification project (linuxtesting.org). > >> > >> Signed-off-by: Anton Vasilyev <vasilyev@ispras.ru> > >> --- > >> drivers/infiniband/hw/ocrdma/ocrdma_stats.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/drivers/infiniband/hw/ocrdma/ocrdma_stats.c b/drivers/infiniband/hw/ocrdma/ocrdma_stats.c > >> index 66056f9..d42c617 100644 > >> --- a/drivers/infiniband/hw/ocrdma/ocrdma_stats.c > >> +++ b/drivers/infiniband/hw/ocrdma/ocrdma_stats.c > >> @@ -834,7 +834,7 @@ void ocrdma_add_port_stats(struct ocrdma_dev *dev) > >> > >> dev->reset_stats.type = OCRDMA_RESET_STATS; > >> dev->reset_stats.dev = dev; > >> - if (!debugfs_create_file("reset_stats", S_IRUSR, dev->dir, > >> + if (!debugfs_create_file("reset_stats", S_IWUSR, dev->dir, > >> &dev->reset_stats, &ocrdma_dbg_ops)) > >> goto err; > >> > > > > If I didn't miss anything, the reset_stats knob wouldn't work. > > > > Any read of statistics from that debugfs will trigger the call to > > ocrdma_update_stats and it will call to ocrdma_mbx_rdma_stats(dev, false). > > > > For write operations to that "reset_stats", there is call > > to ocrdma_mbx_rdma_stats with second argument (reset) as true. > > > > But it doesn't do much with that "reset" argument, except update of reset_stats. > > > > git grep reset_stats drivers/infiniband/hw/ocrdma/* drivers/net/ethernet/emulex/ > > drivers/infiniband/hw/ocrdma/ocrdma.h: struct ocrdma_stats reset_stats; > > drivers/infiniband/hw/ocrdma/ocrdma_hw.c: req->reset_stats = reset; > > "reset" filed is passed down to the FW mailbox command to reset the > stats maintained by FW. > req is a pointer to dev->stats_mem.va and this is used to send down > the command to FW. > status = ocrdma_nonemb_mbx_cmd(dev, mqe, dev->stats_mem.va); It still doesn't make a lot of sense to me: ocrdma_mbx_rdma_stats(): 1315 if (reset) 1316 req->reset_stats = reset; 1317 1318 status = ocrdma_nonemb_mbx_cmd(dev, mqe, dev->stats_mem.va); 1117 static int ocrdma_nonemb_mbx_cmd(struct ocrdma_dev *dev, struct ocrdma_mqe *mqe, 1118 void *payload_va) 1119 { 1120 int status; 1121 struct ocrdma_mbx_rsp *rsp = payload_va; 1122 1123 if ((mqe->hdr.spcl_sge_cnt_emb & OCRDMA_MQE_HDR_EMB_MASK) 1124 OCRDMA_MQE_HDR_EMB_SHIFT) 1125 BUG(); 1126 1127 status = ocrdma_mbx_cmd(dev, mqe); You are not using rsp and/or payload_va to provide data to ocrdma_mbx_cmd. How are you passing reset_stats information to FW? Thanks > > > > drivers/infiniband/hw/ocrdma/ocrdma_sli.h: u8 reset_stats; > > drivers/infiniband/hw/ocrdma/ocrdma_stats.c: dev->reset_stats.type = OCRDMA_RESET_STATS; > > drivers/infiniband/hw/ocrdma/ocrdma_stats.c: dev->reset_stats.dev = dev; > > drivers/infiniband/hw/ocrdma/ocrdma_stats.c: if (!debugfs_create_file("reset_stats", S_IRUSR, dev->dir, > > drivers/infiniband/hw/ocrdma/ocrdma_stats.c: &dev->reset_stats, &ocrdma_dbg_ops)) > > drivers/net/ethernet/emulex/benet/be_cmds.c: req->cmd_params.params.reset_stats = 0; > > drivers/net/ethernet/emulex/benet/be_cmds.h: u8 reset_stats; > > > > Thanks > > The patch looks good to me. > Acked-by: Selvin Xavier <selvin.xavier@broadcom.com> > > Thanks > Selvin [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] hw: Fix permissions for OCRDMA_RESET_STATS 2017-12-19 12:30 ` Leon Romanovsky @ 2017-12-19 17:44 ` Selvin Xavier 2017-12-21 5:05 ` Leon Romanovsky 0 siblings, 1 reply; 7+ messages in thread From: Selvin Xavier @ 2017-12-19 17:44 UTC (permalink / raw) To: Leon Romanovsky Cc: Anton Vasilyev, Devesh Sharma, Doug Ledford, Sean Hefty, Hal Rosenstock, Michael Mera, linux-rdma, linux-kernel, ldv-project On Tue, Dec 19, 2017 at 6:00 PM, Leon Romanovsky <leon@kernel.org> wrote: >> status = ocrdma_nonemb_mbx_cmd(dev, mqe, dev->stats_mem.va); > > It still doesn't make a lot of sense to me: > > ocrdma_mbx_rdma_stats(): > 1315 if (reset) > 1316 req->reset_stats = reset; > 1317 > 1318 status = ocrdma_nonemb_mbx_cmd(dev, mqe, dev->stats_mem.va); > > > 1117 static int ocrdma_nonemb_mbx_cmd(struct ocrdma_dev *dev, struct ocrdma_mqe *mqe, > 1118 void *payload_va) > 1119 { > 1120 int status; > 1121 struct ocrdma_mbx_rsp *rsp = payload_va; > 1122 > 1123 if ((mqe->hdr.spcl_sge_cnt_emb & OCRDMA_MQE_HDR_EMB_MASK) > 1124 OCRDMA_MQE_HDR_EMB_SHIFT) > 1125 BUG(); > 1126 > 1127 status = ocrdma_mbx_cmd(dev, mqe); > > You are not using rsp and/or payload_va to provide data to ocrdma_mbx_cmd. > > How are you passing reset_stats information to FW? > This is a command where the response size is more than 236 bytes. So this is sent as a non-embedded command where the req/resp will be available in payload_va and phys address of payload_va (payload_pa) will be send down to firmware as MQE. FW will dma the mailbox from the payload_pa and response is transferred back. req->reset_stats = reset; is actually setting the corresponding bit in the request structure @payload_va. FW receives reset_stats once the request is DMA-ed from payload_pa. > Thanks > Thanks, Selvin ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] hw: Fix permissions for OCRDMA_RESET_STATS 2017-12-19 17:44 ` Selvin Xavier @ 2017-12-21 5:05 ` Leon Romanovsky 0 siblings, 0 replies; 7+ messages in thread From: Leon Romanovsky @ 2017-12-21 5:05 UTC (permalink / raw) To: Selvin Xavier Cc: Anton Vasilyev, Devesh Sharma, Doug Ledford, Sean Hefty, Hal Rosenstock, Michael Mera, linux-rdma, linux-kernel, ldv-project [-- Attachment #1: Type: text/plain, Size: 1662 bytes --] On Tue, Dec 19, 2017 at 11:14:03PM +0530, Selvin Xavier wrote: > On Tue, Dec 19, 2017 at 6:00 PM, Leon Romanovsky <leon@kernel.org> wrote: > >> status = ocrdma_nonemb_mbx_cmd(dev, mqe, dev->stats_mem.va); > > > > It still doesn't make a lot of sense to me: > > > > ocrdma_mbx_rdma_stats(): > > 1315 if (reset) > > 1316 req->reset_stats = reset; > > 1317 > > 1318 status = ocrdma_nonemb_mbx_cmd(dev, mqe, dev->stats_mem.va); > > > > > > 1117 static int ocrdma_nonemb_mbx_cmd(struct ocrdma_dev *dev, struct ocrdma_mqe *mqe, > > 1118 void *payload_va) > > 1119 { > > 1120 int status; > > 1121 struct ocrdma_mbx_rsp *rsp = payload_va; > > 1122 > > 1123 if ((mqe->hdr.spcl_sge_cnt_emb & OCRDMA_MQE_HDR_EMB_MASK) > > 1124 OCRDMA_MQE_HDR_EMB_SHIFT) > > 1125 BUG(); > > 1126 > > 1127 status = ocrdma_mbx_cmd(dev, mqe); > > > > You are not using rsp and/or payload_va to provide data to ocrdma_mbx_cmd. > > > > How are you passing reset_stats information to FW? > > > This is a command where the response size is more than 236 bytes. So this is > sent as a non-embedded command where the req/resp will be available in > payload_va > and phys address of payload_va (payload_pa) will be send down to > firmware as MQE. FW will dma the mailbox > from the payload_pa and response is transferred back. > > req->reset_stats = reset; is actually setting the corresponding bit in > the request structure @payload_va. > FW receives reset_stats once the request is DMA-ed from payload_pa. Thanks > > > Thanks > > > > Thanks, > Selvin [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: hw: Fix permissions for OCRDMA_RESET_STATS 2017-08-08 15:56 [PATCH] hw: Fix permissions for OCRDMA_RESET_STATS Anton Vasilyev 2017-08-15 18:31 ` Leon Romanovsky @ 2017-12-21 2:59 ` Jason Gunthorpe 1 sibling, 0 replies; 7+ messages in thread From: Jason Gunthorpe @ 2017-12-21 2:59 UTC (permalink / raw) To: Anton Vasilyev Cc: Selvin Xavier, Devesh Sharma, Doug Ledford, Sean Hefty, Hal Rosenstock, Leon Romanovsky, Michael Mera, linux-rdma, linux-kernel, ldv-project On Tue, Aug 08, 2017 at 06:56:37PM +0300, Anton Vasilyev wrote: > Debugfs file reset_stats is created with S_IRUSR permissions, > but ocrdma_dbgfs_ops_read() doesn't support OCRDMA_RESET_STATS, > whereas ocrdma_dbgfs_ops_write() supports only OCRDMA_RESET_STATS. > > The patch fixes misstype with permissions. > > Found by Linux Driver Verification project (linuxtesting.org). > > Signed-off-by: Anton Vasilyev <vasilyev@ispras.ru> > Acked-by: Selvin Xavier <selvin.xavier@broadcom.com> > drivers/infiniband/hw/ocrdma/ocrdma_stats.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/infiniband/hw/ocrdma/ocrdma_stats.c b/drivers/infiniband/hw/ocrdma/ocrdma_stats.c > index 66056f9..d42c617 100644 > +++ b/drivers/infiniband/hw/ocrdma/ocrdma_stats.c > @@ -834,7 +834,7 @@ void ocrdma_add_port_stats(struct ocrdma_dev *dev) > > dev->reset_stats.type = OCRDMA_RESET_STATS; > dev->reset_stats.dev = dev; > - if (!debugfs_create_file("reset_stats", S_IRUSR, dev->dir, > + if (!debugfs_create_file("reset_stats", S_IWUSR, dev->dir, > &dev->reset_stats, &ocrdma_dbg_ops)) > goto err; > I changed the S_IWUSR to 0200 to make checkpatch happy, slowly we are trying to move to the standard style throughout, but applied to for-next now. Thanks, Jason ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2017-12-21 5:05 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-08-08 15:56 [PATCH] hw: Fix permissions for OCRDMA_RESET_STATS Anton Vasilyev 2017-08-15 18:31 ` Leon Romanovsky 2017-12-19 10:29 ` Selvin Xavier 2017-12-19 12:30 ` Leon Romanovsky 2017-12-19 17:44 ` Selvin Xavier 2017-12-21 5:05 ` Leon Romanovsky 2017-12-21 2:59 ` Jason Gunthorpe
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox