On Tue, Sep 22, 2020 at 05:19:06PM +0800, Jing Xiangfeng wrote:
>
>
> On 2020/9/22 16:04, Dan Carpenter wrote:
> > On Tue, Sep 22, 2020 at 03:25:25PM +0800, Jing Xiangfeng wrote:
> > > rio_mport_add_riodev() misses to call put_device() when the device
> > > already exists. Add the missed function call to fix it.
> > >
> > Looks good.
> >
> > Reviewed-by: Dan Carpenter <dan.carpen...@oracle.com>
> >
> > I notice that rio_mport_del_riodev() has a related bug.
> >
> > 1802 err = rio_add_device(rdev);
> > 1803 if (err)
> > 1804 goto cleanup;
> > 1805 rio_dev_get(rdev);
> > ^^^^^^^^^^^^^^^^^
> > This calls get_device(&rdev->dev);
> >
> > 1806
> > 1807 return 0;
> > 1808 cleanup:
> > 1809 kfree(rdev);
> > 1810 return err;
> > 1811 }
> > 1812
> > 1813 static int rio_mport_del_riodev(struct mport_cdev_priv *priv, void
> > __user *arg)
> > 1814 {
> > 1815 struct rio_rdev_info dev_info;
> > 1816 struct rio_dev *rdev = NULL;
> > 1817 struct device *dev;
> > 1818 struct rio_mport *mport;
> > 1819 struct rio_net *net;
> > 1820
> > 1821 if (copy_from_user(&dev_info, arg, sizeof(dev_info)))
> > 1822 return -EFAULT;
> > 1823 dev_info.name[sizeof(dev_info.name) - 1] = '\0';
> > 1824
> > 1825 mport = priv->md->mport;
> > 1826
> > 1827 /* If device name is specified, removal by name has
> > priority */
> > 1828 if (strlen(dev_info.name)) {
> > 1829 dev = bus_find_device_by_name(&rio_bus_type, NULL,
> > 1830 dev_info.name);
> > 1831 if (dev)
> > 1832 rdev = to_rio_dev(dev);
> >
> > This path takes a second get_device(&rdev->dev);
> >
> > 1833 } else {
> > 1834 do {
> > 1835 rdev = rio_get_comptag(dev_info.comptag,
> > rdev);
> > 1836 if (rdev && rdev->dev.parent ==
> > &mport->net->dev &&
> > 1837 rdev->destid == dev_info.destid &&
> > 1838 rdev->hopcount == dev_info.hopcount)
> > 1839 break;
> >
> > This path does not call get_device().
> Add calling rio_dev_get() in this path? like the following changes:
>
> static int rio_mport_del_riodev(struct mport_cdev_priv *priv, void __user
> *arg)
> rdev = rio_get_comptag(dev_info.comptag, rdev);
> if (rdev && rdev->dev.parent == &mport->net->dev &&
> rdev->destid == dev_info.destid &&
> - rdev->hopcount == dev_info.hopcount)
> + rdev->hopcount == dev_info.hopcount) {
> + rio_dev_get(rdev);
> break;
> + }
To be honest, I'm not sure how the rio_get_comptag() stuff is supposed
to work... It probably requires some thought and testing.
Anyway, your patch is straight forward enough so let's just merge that
and hope someone with the hardware can take a look at this.
regards,
dan carpenter
