diff for duplicates of <18cf439a-8fde-02b0-31b6-9ac42f7e972c@broadcom.com> diff --git a/a/1.txt b/N1/1.txt index 860ef8e..c1bd5a6 100644 --- a/a/1.txt +++ b/N1/1.txt @@ -1,78 +1,76 @@ - -On 8/8/2020 7:47 AM, Florian Fainelli wrote: -> -> -> On 8/7/2020 8:55 PM, Dhananjay Phadke wrote: ->> On 8/7/2020, Florian Fainelli wrote: ->>>> When i2c client unregisters, synchronize irq before setting ->>>> iproc_i2c->slave to NULL. +On 8/10/2020 02:17 PM, Ray Jui wrote: +>> On 8/7/2020 8:55 PM, Dhananjay Phadke wrote: +>>> On 8/7/2020, Florian Fainelli wrote: +>>>>> When i2c client unregisters, synchronize irq before setting +>>>>> iproc_i2c->slave to NULL. +>>>>> +>>>>> (1) disable_irq() +>>>>> (2) Mask event enable bits in control reg +>>>>> (3) Erase slave address (avoid further writes to rx fifo) +>>>>> (4) Flush tx and rx FIFOs +>>>>> (5) Clear pending event (interrupt) bits in status reg +>>>>> (6) enable_irq() +>>>>> (7) Set client pointer to NULL +>>>>> >>>> ->>>> (1) disable_irq() ->>>> (2) Mask event enable bits in control reg ->>>> (3) Erase slave address (avoid further writes to rx fifo) ->>>> (4) Flush tx and rx FIFOs ->>>> (5) Clear pending event (interrupt) bits in status reg ->>>> (6) enable_irq() ->>>> (7) Set client pointer to NULL +>>>>> @@ -1091,6 +1091,17 @@ static int bcm_iproc_i2c_unreg_slave(struct i2c_client *slave) +>>>>> tmp &= ~BIT(S_CFG_EN_NIC_SMB_ADDR3_SHIFT); +>>>>> iproc_i2c_wr_reg(iproc_i2c, S_CFG_SMBUS_ADDR_OFFSET, tmp); +>>>>> +>>>>> + /* flush TX/RX FIFOs */ +>>>>> + tmp = (BIT(S_FIFO_RX_FLUSH_SHIFT) | BIT(S_FIFO_TX_FLUSH_SHIFT)); +>>>>> + iproc_i2c_wr_reg(iproc_i2c, S_FIFO_CTRL_OFFSET, tmp); +>>>>> + +>>>>> + /* clear all pending slave interrupts */ +>>>>> + iproc_i2c_wr_reg(iproc_i2c, IS_OFFSET, ISR_MASK_SLAVE); +>>>>> + +>>>>> + enable_irq(iproc_i2c->irq); +>>>>> + +>>>>> + iproc_i2c->slave = NULL; >>>> +>>>> There is nothing that checks on iproc_i2c->slave being valid within the +>>>> interrupt handler, we assume that the pointer is valid which is fin, +>>>> however non functional it may be, it may feel more natural to move the +>>>> assignment before the enable_irq()? >>> ->>>> @@ -1091,6 +1091,17 @@ static int bcm_iproc_i2c_unreg_slave(struct i2c_client *slave) ->>>> tmp &= ~BIT(S_CFG_EN_NIC_SMB_ADDR3_SHIFT); ->>>> iproc_i2c_wr_reg(iproc_i2c, S_CFG_SMBUS_ADDR_OFFSET, tmp); ->>>> ->>>> + /* flush TX/RX FIFOs */ ->>>> + tmp = (BIT(S_FIFO_RX_FLUSH_SHIFT) | BIT(S_FIFO_TX_FLUSH_SHIFT)); ->>>> + iproc_i2c_wr_reg(iproc_i2c, S_FIFO_CTRL_OFFSET, tmp); ->>>> + ->>>> + /* clear all pending slave interrupts */ ->>>> + iproc_i2c_wr_reg(iproc_i2c, IS_OFFSET, ISR_MASK_SLAVE); ->>>> + ->>>> + enable_irq(iproc_i2c->irq); ->>>> + ->>>> + iproc_i2c->slave = NULL; +>>> As far as the teardown sequence ensures no more interrupts arrive after +>>> enable_irq() and they are enabled only after setting pointer during +>>> client register(); checking for NULL in ISR isn't necessary. +>> +>> Agreed. +>> +> +>Okay I think we all agree that this teardown sequence will guarantee +>that no further "slave" interrupts will be fired after it. +> >>> ->>> There is nothing that checks on iproc_i2c->slave being valid within the ->>> interrupt handler, we assume that the pointer is valid which is fin, ->>> however non functional it may be, it may feel more natural to move the ->>> assignment before the enable_irq()? ->> ->> As far as the teardown sequence ensures no more interrupts arrive after ->> enable_irq() and they are enabled only after setting pointer during ->> client register(); checking for NULL in ISR isn't necessary. -> -> Agreed. -> - -Okay I think we all agree that this teardown sequence will guarantee -that no further "slave" interrupts will be fired after it. - ->> ->> If The teardown sequence doesn't guarantee quiescing of interrupts, ->> setting NULL before or after enable_irq() is equally vulnerable. -> -> The teardown sequence is sort of a critical section if we may say, so -> ensuring that everything happens within it and that enable_irq() is the -> last operation would seem more natural to me at least. Thanks -> - -I tend to agree with Florian here. +>>> If The teardown sequence doesn't guarantee quiescing of interrupts, +>>> setting NULL before or after enable_irq() is equally vulnerable. +>> +>> The teardown sequence is sort of a critical section if we may say, so +>> ensuring that everything happens within it and that enable_irq() is the +>> last operation would seem more natural to me at least. Thanks +>> +> +>I tend to agree with Florian here. +> +>1. Enable/Disable IRQ is done on the interrupt line for both master and +>slave (or even other peripherals that share the same interrupt line, +>although that is not the case here since this interrupt is dedicated to +>I2C in all iProc based SoCs). +> +>2. The tear down sequence here wrapped by disable/enable_irq is slave +>specific +> +>The effect of 1. is temporary, and the purpose of it is to ensure slave +>interrupts are quiesced properly at the end of the sequence. +> +>If we consider both 1. and 2., I agree with Florian that while the end +>result is the same, it is indeed more natural to wrap the entire slave +>tear down sequence within disable/enable irq. -1. Enable/Disable IRQ is done on the interrupt line for both master and -slave (or even other peripherals that share the same interrupt line, -although that is not the case here since this interrupt is dedicated to -I2C in all iProc based SoCs). - -2. The tear down sequence here wrapped by disable/enable_irq is slave -specific - -The effect of 1. is temporary, and the purpose of it is to ensure slave -interrupts are quiesced properly at the end of the sequence. - -If we consider both 1. and 2., I agree with Florian that while the end -result is the same, it is indeed more natural to wrap the entire slave -tear down sequence within disable/enable irq. +Ok, will send v3 with this change. Thanks, - -Ray +Dhananjay diff --git a/a/content_digest b/N1/content_digest index 303ac7c..b26798f 100644 --- a/a/content_digest +++ b/N1/content_digest @@ -1,12 +1,10 @@ - "ref\000a30ca7-d533-94ba-994a-9a133fadb045@gmail.com\0" - "ref\01596858925-45763-1-git-send-email-dphadke@linux.microsoft.com\0" - "ref\00f7063f5-8ca7-f469-574b-82382d49e266@gmail.com\0" - "From\0Ray Jui <ray.jui@broadcom.com>\0" + "From\0Dhananjay Phadke <dphadke@linux.microsoft.com>\0" "Subject\0Re: [PATCH v2] i2c: iproc: fix race between client unreg and isr\0" - "Date\0Mon, 10 Aug 2020 14:17:10 -0700\0" - "To\0Florian Fainelli <f.fainelli@gmail.com>" - " Dhananjay Phadke <dphadke@linux.microsoft.com>\0" + "Date\0Mon, 10 Aug 2020 15:22:49 -0700\0" + "To\0ray.jui@broadcom.com\0" "Cc\0bcm-kernel-feedback-list@broadcom.com" + dphadke@linux.microsoft.com + f.fainelli@gmail.com linux-i2c@vger.kernel.org linux-kernel@vger.kernel.org rayagonda.kokatanur@broadcom.com @@ -15,82 +13,80 @@ "\00:1\0" "b\0" "\n" - "\n" - "On 8/8/2020 7:47 AM, Florian Fainelli wrote:\n" - "> \n" - "> \n" - "> On 8/7/2020 8:55 PM, Dhananjay Phadke wrote:\n" - ">> On 8/7/2020, Florian Fainelli wrote:\n" - ">>>> When i2c client unregisters, synchronize irq before setting\n" - ">>>> iproc_i2c->slave to NULL.\n" + "On 8/10/2020 02:17 PM, Ray Jui wrote:\n" + ">> On 8/7/2020 8:55 PM, Dhananjay Phadke wrote:\n" + ">>> On 8/7/2020, Florian Fainelli wrote:\n" + ">>>>> When i2c client unregisters, synchronize irq before setting\n" + ">>>>> iproc_i2c->slave to NULL.\n" + ">>>>>\n" + ">>>>> (1) disable_irq()\n" + ">>>>> (2) Mask event enable bits in control reg\n" + ">>>>> (3) Erase slave address (avoid further writes to rx fifo)\n" + ">>>>> (4) Flush tx and rx FIFOs\n" + ">>>>> (5) Clear pending event (interrupt) bits in status reg\n" + ">>>>> (6) enable_irq()\n" + ">>>>> (7) Set client pointer to NULL\n" + ">>>>>\n" ">>>>\n" - ">>>> (1) disable_irq()\n" - ">>>> (2) Mask event enable bits in control reg\n" - ">>>> (3) Erase slave address (avoid further writes to rx fifo)\n" - ">>>> (4) Flush tx and rx FIFOs\n" - ">>>> (5) Clear pending event (interrupt) bits in status reg\n" - ">>>> (6) enable_irq()\n" - ">>>> (7) Set client pointer to NULL\n" + ">>>>> @@ -1091,6 +1091,17 @@ static int bcm_iproc_i2c_unreg_slave(struct i2c_client *slave)\n" + ">>>>> \ttmp &= ~BIT(S_CFG_EN_NIC_SMB_ADDR3_SHIFT);\n" + ">>>>> \tiproc_i2c_wr_reg(iproc_i2c, S_CFG_SMBUS_ADDR_OFFSET, tmp);\n" + ">>>>> \n" + ">>>>> +\t/* flush TX/RX FIFOs */\n" + ">>>>> +\ttmp = (BIT(S_FIFO_RX_FLUSH_SHIFT) | BIT(S_FIFO_TX_FLUSH_SHIFT));\n" + ">>>>> +\tiproc_i2c_wr_reg(iproc_i2c, S_FIFO_CTRL_OFFSET, tmp);\n" + ">>>>> +\n" + ">>>>> +\t/* clear all pending slave interrupts */\n" + ">>>>> +\tiproc_i2c_wr_reg(iproc_i2c, IS_OFFSET, ISR_MASK_SLAVE);\n" + ">>>>> +\n" + ">>>>> +\tenable_irq(iproc_i2c->irq);\n" + ">>>>> +\n" + ">>>>> +\tiproc_i2c->slave = NULL;\n" ">>>>\n" + ">>>> There is nothing that checks on iproc_i2c->slave being valid within the\n" + ">>>> interrupt handler, we assume that the pointer is valid which is fin,\n" + ">>>> however non functional it may be, it may feel more natural to move the\n" + ">>>> assignment before the enable_irq()?\n" ">>>\n" - ">>>> @@ -1091,6 +1091,17 @@ static int bcm_iproc_i2c_unreg_slave(struct i2c_client *slave)\n" - ">>>> \ttmp &= ~BIT(S_CFG_EN_NIC_SMB_ADDR3_SHIFT);\n" - ">>>> \tiproc_i2c_wr_reg(iproc_i2c, S_CFG_SMBUS_ADDR_OFFSET, tmp);\n" - ">>>> \n" - ">>>> +\t/* flush TX/RX FIFOs */\n" - ">>>> +\ttmp = (BIT(S_FIFO_RX_FLUSH_SHIFT) | BIT(S_FIFO_TX_FLUSH_SHIFT));\n" - ">>>> +\tiproc_i2c_wr_reg(iproc_i2c, S_FIFO_CTRL_OFFSET, tmp);\n" - ">>>> +\n" - ">>>> +\t/* clear all pending slave interrupts */\n" - ">>>> +\tiproc_i2c_wr_reg(iproc_i2c, IS_OFFSET, ISR_MASK_SLAVE);\n" - ">>>> +\n" - ">>>> +\tenable_irq(iproc_i2c->irq);\n" - ">>>> +\n" - ">>>> +\tiproc_i2c->slave = NULL;\n" + ">>> As far as the teardown sequence ensures no more interrupts arrive after\n" + ">>> enable_irq() and they are enabled only after setting pointer during\n" + ">>> client register(); checking for NULL in ISR isn't necessary. \n" + ">> \n" + ">> Agreed.\n" + ">> \n" + ">\n" + ">Okay I think we all agree that this teardown sequence will guarantee\n" + ">that no further \"slave\" interrupts will be fired after it.\n" + ">\n" ">>>\n" - ">>> There is nothing that checks on iproc_i2c->slave being valid within the\n" - ">>> interrupt handler, we assume that the pointer is valid which is fin,\n" - ">>> however non functional it may be, it may feel more natural to move the\n" - ">>> assignment before the enable_irq()?\n" - ">>\n" - ">> As far as the teardown sequence ensures no more interrupts arrive after\n" - ">> enable_irq() and they are enabled only after setting pointer during\n" - ">> client register(); checking for NULL in ISR isn't necessary. \n" - "> \n" - "> Agreed.\n" - "> \n" - "\n" - "Okay I think we all agree that this teardown sequence will guarantee\n" - "that no further \"slave\" interrupts will be fired after it.\n" - "\n" - ">>\n" - ">> If The teardown sequence doesn't guarantee quiescing of interrupts,\n" - ">> setting NULL before or after enable_irq() is equally vulnerable.\n" - "> \n" - "> The teardown sequence is sort of a critical section if we may say, so\n" - "> ensuring that everything happens within it and that enable_irq() is the\n" - "> last operation would seem more natural to me at least. Thanks\n" - "> \n" - "\n" - "I tend to agree with Florian here.\n" + ">>> If The teardown sequence doesn't guarantee quiescing of interrupts,\n" + ">>> setting NULL before or after enable_irq() is equally vulnerable.\n" + ">> \n" + ">> The teardown sequence is sort of a critical section if we may say, so\n" + ">> ensuring that everything happens within it and that enable_irq() is the\n" + ">> last operation would seem more natural to me at least. Thanks\n" + ">> \n" + ">\n" + ">I tend to agree with Florian here.\n" + ">\n" + ">1. Enable/Disable IRQ is done on the interrupt line for both master and\n" + ">slave (or even other peripherals that share the same interrupt line,\n" + ">although that is not the case here since this interrupt is dedicated to\n" + ">I2C in all iProc based SoCs).\n" + ">\n" + ">2. The tear down sequence here wrapped by disable/enable_irq is slave\n" + ">specific\n" + ">\n" + ">The effect of 1. is temporary, and the purpose of it is to ensure slave\n" + ">interrupts are quiesced properly at the end of the sequence.\n" + ">\n" + ">If we consider both 1. and 2., I agree with Florian that while the end\n" + ">result is the same, it is indeed more natural to wrap the entire slave\n" + ">tear down sequence within disable/enable irq.\n" "\n" - "1. Enable/Disable IRQ is done on the interrupt line for both master and\n" - "slave (or even other peripherals that share the same interrupt line,\n" - "although that is not the case here since this interrupt is dedicated to\n" - "I2C in all iProc based SoCs).\n" - "\n" - "2. The tear down sequence here wrapped by disable/enable_irq is slave\n" - "specific\n" - "\n" - "The effect of 1. is temporary, and the purpose of it is to ensure slave\n" - "interrupts are quiesced properly at the end of the sequence.\n" - "\n" - "If we consider both 1. and 2., I agree with Florian that while the end\n" - "result is the same, it is indeed more natural to wrap the entire slave\n" - "tear down sequence within disable/enable irq.\n" + "Ok, will send v3 with this change.\n" "\n" "Thanks,\n" - "\n" - Ray + Dhananjay -37cb4476391c3554e11048e4417eee524393f1d0c3263eea588bd154f99c3907 +a684dde5c183b1be6ecffc2aaa4fbb55c2b2d2c7b9cbb57f88486f976dec8da0
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox