| Message ID | 20211001050228.55183-14-takahiro.akashi@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | efi_loader: more tightly integrate UEFI disks to device model | expand |
On Thu, 30 Sept 2021 at 23:03, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote: > > Every time an ide bus/port is scanned and a new device is detected, > we want to call device_probe() as it will give us a chance to run additional > post-processings for some purposes. > > In particular, support for creating partitions on a device will be added. > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > --- > drivers/block/ide.c | 6 ++++++ > 1 file changed, 6 insertions(+) > Reviewed-by: Simon Glass <sjg@chromium.org> I'm starting to wonder if you can create a function that does the probe and unbind? Something in the blk interface, perhaps? It would reduce the duplicated code and provide a standard way of bringing up a new device. Perhaps blk_device_complete() ? Regards, Simon
On Sun, Oct 10, 2021 at 08:14:13AM -0600, Simon Glass wrote: > On Thu, 30 Sept 2021 at 23:03, AKASHI Takahiro > <takahiro.akashi@linaro.org> wrote: > > > > Every time an ide bus/port is scanned and a new device is detected, > > we want to call device_probe() as it will give us a chance to run additional > > post-processings for some purposes. > > > > In particular, support for creating partitions on a device will be added. > > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > --- > > drivers/block/ide.c | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > Reviewed-by: Simon Glass <sjg@chromium.org> > > I'm starting to wonder if you can create a function that does the > probe and unbind? Something in the blk interface, perhaps? It would > reduce the duplicated code and provide a standard way of bringing up a > new device. That is exactly what Ilias suggested but I'm a bit declined to do :) Common 'scanning' code looks like: blk_create_devicef(... , &dev); desc = dev_get_uclass_data(dev); initialize some members in desc as well as device-specific info --- (A) (now dev can be accessible.) ret = device_probe(dev); if (ret) { de-initialize *dev* --- (B) device_unbind() } Basically (B) is supposed to undo (A) which may or may not exist, depending on types of block devices. So I'm not 100% sure that a combination of device_probe() and device_unbind() will fit to all the device types. (The only cases that I have noticed are fsl_sata.c and sata_sil.c. Both have their own xxx_unbind_device(), but they simply call device_remove() and device_unbind(), though. So no worry?) -Takahiro Akashi > Perhaps blk_device_complete() ? > > Regards, > Simon
Hi Takahiro, On Sun, 10 Oct 2021 at 19:43, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote: > > On Sun, Oct 10, 2021 at 08:14:13AM -0600, Simon Glass wrote: > > On Thu, 30 Sept 2021 at 23:03, AKASHI Takahiro > > <takahiro.akashi@linaro.org> wrote: > > > > > > Every time an ide bus/port is scanned and a new device is detected, > > > we want to call device_probe() as it will give us a chance to run additional > > > post-processings for some purposes. > > > > > > In particular, support for creating partitions on a device will be added. > > > > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > > --- > > > drivers/block/ide.c | 6 ++++++ > > > 1 file changed, 6 insertions(+) > > > > > > > Reviewed-by: Simon Glass <sjg@chromium.org> > > > > I'm starting to wonder if you can create a function that does the > > probe and unbind? Something in the blk interface, perhaps? It would > > reduce the duplicated code and provide a standard way of bringing up a > > new device. > > That is exactly what Ilias suggested but I'm a bit declined to do :) > > Common 'scanning' code looks like: > blk_create_devicef(... , &dev); > desc = dev_get_uclass_data(dev); > initialize some members in desc as well as device-specific info --- (A) > (now dev can be accessible.) > ret = device_probe(dev); > if (ret) { > de-initialize *dev* --- (B) > device_unbind() > } > > Basically (B) is supposed to undo (A) which may or may not exist, > depending on types of block devices. > > So I'm not 100% sure that a combination of device_probe() and device_unbind() > will fit to all the device types. > (The only cases that I have noticed are fsl_sata.c and sata_sil.c. Both > have their own xxx_unbind_device(), but they simply call device_remove() and > device_unbind(), though. So no worry?) Yes I agree it would be a very strange function. But at least it would have the benefit of grouping the code together under a particular name, something like blk_back_out_bind(), but that's not a good name....it just feels like this might get refactored in the future and having the code in one place might be handy. Regards, Simon
On Mon, Oct 11, 2021 at 08:54:13AM -0600, Simon Glass wrote: > Hi Takahiro, > > On Sun, 10 Oct 2021 at 19:43, AKASHI Takahiro > <takahiro.akashi@linaro.org> wrote: > > > > On Sun, Oct 10, 2021 at 08:14:13AM -0600, Simon Glass wrote: > > > On Thu, 30 Sept 2021 at 23:03, AKASHI Takahiro > > > <takahiro.akashi@linaro.org> wrote: > > > > > > > > Every time an ide bus/port is scanned and a new device is detected, > > > > we want to call device_probe() as it will give us a chance to run additional > > > > post-processings for some purposes. > > > > > > > > In particular, support for creating partitions on a device will be added. > > > > > > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > > > --- > > > > drivers/block/ide.c | 6 ++++++ > > > > 1 file changed, 6 insertions(+) > > > > > > > > > > Reviewed-by: Simon Glass <sjg@chromium.org> > > > > > > I'm starting to wonder if you can create a function that does the > > > probe and unbind? Something in the blk interface, perhaps? It would > > > reduce the duplicated code and provide a standard way of bringing up a > > > new device. > > > > That is exactly what Ilias suggested but I'm a bit declined to do :) > > > > Common 'scanning' code looks like: > > blk_create_devicef(... , &dev); > > desc = dev_get_uclass_data(dev); > > initialize some members in desc as well as device-specific info --- (A) > > (now dev can be accessible.) > > ret = device_probe(dev); > > if (ret) { > > de-initialize *dev* --- (B) > > device_unbind() > > } > > > > Basically (B) is supposed to undo (A) which may or may not exist, > > depending on types of block devices. > > > > So I'm not 100% sure that a combination of device_probe() and device_unbind() > > will fit to all the device types. > > (The only cases that I have noticed are fsl_sata.c and sata_sil.c. Both > > have their own xxx_unbind_device(), but they simply call device_remove() and > > device_unbind(), though. So no worry?) > > Yes I agree it would be a very strange function. But at least it would > have the benefit of grouping the code together under a particular > name, something like blk_back_out_bind(), but that's not a good > name....it just feels like this might get refactored in the future and > having the code in one place might be handy. naming is hard! try_device_probe() maybe? Cheers /Ilias > > Regards, > Simon
On Tue, Oct 12, 2021 at 08:53:54AM +0300, Ilias Apalodimas wrote: > On Mon, Oct 11, 2021 at 08:54:13AM -0600, Simon Glass wrote: > > Hi Takahiro, > > > > On Sun, 10 Oct 2021 at 19:43, AKASHI Takahiro > > <takahiro.akashi@linaro.org> wrote: > > > > > > On Sun, Oct 10, 2021 at 08:14:13AM -0600, Simon Glass wrote: > > > > On Thu, 30 Sept 2021 at 23:03, AKASHI Takahiro > > > > <takahiro.akashi@linaro.org> wrote: > > > > > > > > > > Every time an ide bus/port is scanned and a new device is detected, > > > > > we want to call device_probe() as it will give us a chance to run additional > > > > > post-processings for some purposes. > > > > > > > > > > In particular, support for creating partitions on a device will be added. > > > > > > > > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > > > > --- > > > > > drivers/block/ide.c | 6 ++++++ > > > > > 1 file changed, 6 insertions(+) > > > > > > > > > > > > > Reviewed-by: Simon Glass <sjg@chromium.org> > > > > > > > > I'm starting to wonder if you can create a function that does the > > > > probe and unbind? Something in the blk interface, perhaps? It would > > > > reduce the duplicated code and provide a standard way of bringing up a > > > > new device. > > > > > > That is exactly what Ilias suggested but I'm a bit declined to do :) > > > > > > Common 'scanning' code looks like: > > > blk_create_devicef(... , &dev); > > > desc = dev_get_uclass_data(dev); > > > initialize some members in desc as well as device-specific info --- (A) > > > (now dev can be accessible.) > > > ret = device_probe(dev); > > > if (ret) { > > > de-initialize *dev* --- (B) > > > device_unbind() > > > } > > > > > > Basically (B) is supposed to undo (A) which may or may not exist, > > > depending on types of block devices. > > > > > > So I'm not 100% sure that a combination of device_probe() and device_unbind() > > > will fit to all the device types. > > > (The only cases that I have noticed are fsl_sata.c and sata_sil.c. Both > > > have their own xxx_unbind_device(), but they simply call device_remove() and > > > device_unbind(), though. So no worry?) > > > > Yes I agree it would be a very strange function. But at least it would > > have the benefit of grouping the code together under a particular > > name, something like blk_back_out_bind(), but that's not a good > > name....it just feels like this might get refactored in the future and > > having the code in one place might be handy. > > naming is hard! try_device_probe() maybe? Indeed. A name should come later. So I will temporarily use blk_probe_or_unbind() :-) -Takahiro Akashi > Cheers > /Ilias > > > > Regards, > > Simon
diff --git a/drivers/block/ide.c b/drivers/block/ide.c index c99076c6f45d..31aaed09ab70 100644 --- a/drivers/block/ide.c +++ b/drivers/block/ide.c @@ -1151,6 +1151,12 @@ static int ide_probe(struct udevice *udev) blksz, size, &blk_dev); if (ret) return ret; + + ret = device_probe(blk_dev); + if (ret) { + device_unbind(blk_dev); + return ret; + } } }
Every time an ide bus/port is scanned and a new device is detected, we want to call device_probe() as it will give us a chance to run additional post-processings for some purposes. In particular, support for creating partitions on a device will be added. Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> --- drivers/block/ide.c | 6 ++++++ 1 file changed, 6 insertions(+) -- 2.33.0