| Message ID | 20220712152554.2909224-1-michael@walle.cc (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | NFC: nxp-nci: fix deadlock during firmware update | expand |
On 12/07/2022 17:25, Michael Walle wrote: > During firmware update, both nxp_nci_i2c_irq_thread_fn() and > nxp_nci_fw_work() will hold the info_lock mutex and one will wait > for the other via a completion: > > nxp_nci_fw_work() > mutex_lock(info_lock) > nxp_nci_fw_send() > wait_for_completion(cmd_completion) > mutex_unlock(info_lock) > > nxp_nci_i2c_irq_thread_fn() > mutex_lock(info_lock) > nxp_nci_fw_recv_frame() > complete(cmd_completion) > mutex_unlock(info_lock) > > This will result in a -ETIMEDOUT error during firmware update (note > that the wait_for_completion() above is a variant with a timeout). > > Drop the lock in nxp_nci_fw_work() and instead take it after the > work is done in nxp_nci_fw_work_complete() when the NFC controller mode > is switched and the info structure is updated. > > Fixes: dece45855a8b ("NFC: nxp-nci: Add support for NXP NCI chips") > Signed-off-by: Michael Walle <michael@walle.cc> > --- > drivers/nfc/nxp-nci/firmware.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/drivers/nfc/nxp-nci/firmware.c b/drivers/nfc/nxp-nci/firmware.c > index 119bf305c642..6a4d4aa7239f 100644 > --- a/drivers/nfc/nxp-nci/firmware.c > +++ b/drivers/nfc/nxp-nci/firmware.c > @@ -54,6 +54,7 @@ void nxp_nci_fw_work_complete(struct nxp_nci_info *info, int result) > struct nxp_nci_fw_info *fw_info = &info->fw_info; > int r; > > + mutex_lock(&info->info_lock); > if (info->phy_ops->set_mode) { > r = info->phy_ops->set_mode(info->phy_id, NXP_NCI_MODE_COLD); > if (r < 0 && result == 0) > @@ -66,6 +67,7 @@ void nxp_nci_fw_work_complete(struct nxp_nci_info *info, int result) > release_firmware(fw_info->fw); > fw_info->fw = NULL; > } > + mutex_unlock(&info->info_lock); > > nfc_fw_download_done(info->ndev->nfc_dev, fw_info->name, (u32) -result); > } > @@ -172,8 +174,6 @@ void nxp_nci_fw_work(struct work_struct *work) > fw_info = container_of(work, struct nxp_nci_fw_info, work); > info = container_of(fw_info, struct nxp_nci_info, fw_info); > > - mutex_lock(&info->info_lock); > - I am not sure this is correct. info_lock should protect members of info thus also info->fw_info. By removing the mutex the protection is gone. Unless you are sure that fw_info cannot be modified concurrently? Best regards, Krzysztof
Am 2022-07-13 08:57, schrieb Krzysztof Kozlowski: > On 12/07/2022 17:25, Michael Walle wrote: >> During firmware update, both nxp_nci_i2c_irq_thread_fn() and >> nxp_nci_fw_work() will hold the info_lock mutex and one will wait >> for the other via a completion: >> >> nxp_nci_fw_work() >> mutex_lock(info_lock) >> nxp_nci_fw_send() >> wait_for_completion(cmd_completion) >> mutex_unlock(info_lock) >> >> nxp_nci_i2c_irq_thread_fn() >> mutex_lock(info_lock) >> nxp_nci_fw_recv_frame() >> complete(cmd_completion) >> mutex_unlock(info_lock) >> >> This will result in a -ETIMEDOUT error during firmware update (note >> that the wait_for_completion() above is a variant with a timeout). >> >> Drop the lock in nxp_nci_fw_work() and instead take it after the >> work is done in nxp_nci_fw_work_complete() when the NFC controller >> mode >> is switched and the info structure is updated. >> >> Fixes: dece45855a8b ("NFC: nxp-nci: Add support for NXP NCI chips") >> Signed-off-by: Michael Walle <michael@walle.cc> >> --- >> drivers/nfc/nxp-nci/firmware.c | 5 ++--- >> 1 file changed, 2 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/nfc/nxp-nci/firmware.c >> b/drivers/nfc/nxp-nci/firmware.c >> index 119bf305c642..6a4d4aa7239f 100644 >> --- a/drivers/nfc/nxp-nci/firmware.c >> +++ b/drivers/nfc/nxp-nci/firmware.c >> @@ -54,6 +54,7 @@ void nxp_nci_fw_work_complete(struct nxp_nci_info >> *info, int result) >> struct nxp_nci_fw_info *fw_info = &info->fw_info; >> int r; >> >> + mutex_lock(&info->info_lock); >> if (info->phy_ops->set_mode) { >> r = info->phy_ops->set_mode(info->phy_id, NXP_NCI_MODE_COLD); >> if (r < 0 && result == 0) >> @@ -66,6 +67,7 @@ void nxp_nci_fw_work_complete(struct nxp_nci_info >> *info, int result) >> release_firmware(fw_info->fw); >> fw_info->fw = NULL; >> } >> + mutex_unlock(&info->info_lock); >> >> nfc_fw_download_done(info->ndev->nfc_dev, fw_info->name, (u32) >> -result); >> } >> @@ -172,8 +174,6 @@ void nxp_nci_fw_work(struct work_struct *work) >> fw_info = container_of(work, struct nxp_nci_fw_info, work); >> info = container_of(fw_info, struct nxp_nci_info, fw_info); >> >> - mutex_lock(&info->info_lock); >> - > > I am not sure this is correct. info_lock should protect members of info > thus also info->fw_info. By removing the mutex the protection is gone. > > Unless you are sure that fw_info cannot be modified concurrently? Mh, you are right. fw_info could be modified by the irq thread and in the worker thread (and the nfc core doesn't protect against multiple fw_download() calls) -michael
diff --git a/drivers/nfc/nxp-nci/firmware.c b/drivers/nfc/nxp-nci/firmware.c index 119bf305c642..6a4d4aa7239f 100644 --- a/drivers/nfc/nxp-nci/firmware.c +++ b/drivers/nfc/nxp-nci/firmware.c @@ -54,6 +54,7 @@ void nxp_nci_fw_work_complete(struct nxp_nci_info *info, int result) struct nxp_nci_fw_info *fw_info = &info->fw_info; int r; + mutex_lock(&info->info_lock); if (info->phy_ops->set_mode) { r = info->phy_ops->set_mode(info->phy_id, NXP_NCI_MODE_COLD); if (r < 0 && result == 0) @@ -66,6 +67,7 @@ void nxp_nci_fw_work_complete(struct nxp_nci_info *info, int result) release_firmware(fw_info->fw); fw_info->fw = NULL; } + mutex_unlock(&info->info_lock); nfc_fw_download_done(info->ndev->nfc_dev, fw_info->name, (u32) -result); } @@ -172,8 +174,6 @@ void nxp_nci_fw_work(struct work_struct *work) fw_info = container_of(work, struct nxp_nci_fw_info, work); info = container_of(fw_info, struct nxp_nci_info, fw_info); - mutex_lock(&info->info_lock); - r = fw_info->cmd_result; if (r < 0) goto exit_work; @@ -190,7 +190,6 @@ void nxp_nci_fw_work(struct work_struct *work) exit_work: if (r < 0 || fw_info->size == 0) nxp_nci_fw_work_complete(info, r); - mutex_unlock(&info->info_lock); } int nxp_nci_fw_download(struct nci_dev *ndev, const char *firmware_name)
During firmware update, both nxp_nci_i2c_irq_thread_fn() and nxp_nci_fw_work() will hold the info_lock mutex and one will wait for the other via a completion: nxp_nci_fw_work() mutex_lock(info_lock) nxp_nci_fw_send() wait_for_completion(cmd_completion) mutex_unlock(info_lock) nxp_nci_i2c_irq_thread_fn() mutex_lock(info_lock) nxp_nci_fw_recv_frame() complete(cmd_completion) mutex_unlock(info_lock) This will result in a -ETIMEDOUT error during firmware update (note that the wait_for_completion() above is a variant with a timeout). Drop the lock in nxp_nci_fw_work() and instead take it after the work is done in nxp_nci_fw_work_complete() when the NFC controller mode is switched and the info structure is updated. Fixes: dece45855a8b ("NFC: nxp-nci: Add support for NXP NCI chips") Signed-off-by: Michael Walle <michael@walle.cc> --- drivers/nfc/nxp-nci/firmware.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)