Messages in this thread | | | Date | Wed, 9 Oct 2019 17:40:23 -0700 | From | Jakub Kicinski <> | Subject | Re: [PATCH v9 4/7] nfc: pn533: Split pn533 init & nfc_register |
| |
On Tue, 8 Oct 2019 16:05:41 +0200, Lars Poeschel wrote: > There is a problem in the initialisation and setup of the pn533: It > registers with nfc too early. It could happen, that it finished > registering with nfc and someone starts using it. But setup of the pn533 > is not yet finished. Bad or at least unintended things could happen. > So I split out nfc registering (and unregistering) to seperate functions > that have to be called late in probe then. > > Cc: Johan Hovold <johan@kernel.org> > Cc: Claudiu Beznea <Claudiu.Beznea@microchip.com> > Signed-off-by: Lars Poeschel <poeschel@lemonage.de>
> diff --git a/drivers/nfc/pn533/i2c.c b/drivers/nfc/pn533/i2c.c > index 1abd40398a5a..e9e5a1ec8857 100644 > --- a/drivers/nfc/pn533/i2c.c > +++ b/drivers/nfc/pn533/i2c.c > @@ -193,12 +193,10 @@ static int pn533_i2c_probe(struct i2c_client *client, > phy->i2c_dev = client; > i2c_set_clientdata(client, phy); > > - priv = pn533_register_device(PN533_DEVICE_PN532, > - PN533_NO_TYPE_B_PROTOCOLS, > + priv = pn53x_common_init(PN533_DEVICE_PN532, > PN533_PROTO_REQ_ACK_RESP, > phy, &i2c_phy_ops, NULL, > - &phy->i2c_dev->dev, > - &client->dev); > + &phy->i2c_dev->dev);
nit: start of continuation lines should match the opening parenthesis, please run checkpatch and fix the style issue
> if (IS_ERR(priv)) { > r = PTR_ERR(priv); > @@ -220,13 +218,17 @@ static int pn533_i2c_probe(struct i2c_client *client, > if (r) > goto fn_setup_err; > > - return 0; > + r = pn53x_register_nfc(priv, PN533_NO_TYPE_B_PROTOCOLS, &client->dev); > + if (r) > + goto fn_setup_err; > + > + return r; > > fn_setup_err: > free_irq(client->irq, phy); > > irq_rqst_err: > - pn533_unregister_device(phy->priv); > + pn53x_common_clean(phy->priv); > > return r; > } > @@ -239,7 +241,8 @@ static int pn533_i2c_remove(struct i2c_client *client) > > free_irq(client->irq, phy); > > - pn533_unregister_device(phy->priv); > + pn53x_unregister_nfc(phy->priv); > + pn53x_common_clean(phy->priv); > > return 0; > } > diff --git a/drivers/nfc/pn533/pn533.c b/drivers/nfc/pn533/pn533.c > index 64836c727aee..e5d5e4c83a04 100644 > --- a/drivers/nfc/pn533/pn533.c > +++ b/drivers/nfc/pn533/pn533.c > @@ -2590,14 +2590,12 @@ int pn533_finalize_setup(struct pn533 *dev) > } > EXPORT_SYMBOL_GPL(pn533_finalize_setup); > > -struct pn533 *pn533_register_device(u32 device_type, > - u32 protocols, > +struct pn533 *pn53x_common_init(u32 device_type, > enum pn533_protocol_type protocol_type, > void *phy, > struct pn533_phy_ops *phy_ops, > struct pn533_frame_ops *fops, > - struct device *dev, > - struct device *parent) > + struct device *dev) > { > struct pn533 *priv; > int rc = -ENOMEM; > @@ -2638,43 +2636,18 @@ struct pn533 *pn533_register_device(u32 device_type, > skb_queue_head_init(&priv->fragment_skb); > > INIT_LIST_HEAD(&priv->cmd_queue); > - > - priv->nfc_dev = nfc_allocate_device(&pn533_nfc_ops, protocols, > - priv->ops->tx_header_len + > - PN533_CMD_DATAEXCH_HEAD_LEN, > - priv->ops->tx_tail_len); > - if (!priv->nfc_dev) { > - rc = -ENOMEM; > - goto destroy_wq; > - } > - > - nfc_set_parent_dev(priv->nfc_dev, parent); > - nfc_set_drvdata(priv->nfc_dev, priv); > - > - rc = nfc_register_device(priv->nfc_dev); > - if (rc) > - goto free_nfc_dev;
Aren't you moving too much out of here? Looking at commit 32ecc75ded72 ("NFC: pn533: change order operations in dev registation") it seems like IRQ handler may want to access the data structures, do this change not reintroduce the problem?
> return priv; > > -free_nfc_dev: > - nfc_free_device(priv->nfc_dev); > - > -destroy_wq: > - destroy_workqueue(priv->wq); > error: > kfree(priv); > return ERR_PTR(rc); > } > -EXPORT_SYMBOL_GPL(pn533_register_device); > +EXPORT_SYMBOL_GPL(pn53x_common_init); > > -void pn533_unregister_device(struct pn533 *priv) > +void pn53x_common_clean(struct pn533 *priv) > { > struct pn533_cmd *cmd, *n; > > - nfc_unregister_device(priv->nfc_dev); > - nfc_free_device(priv->nfc_dev); > - > flush_delayed_work(&priv->poll_work); > destroy_workqueue(priv->wq); >
| |