On Thu, Oct 25, 2012 at 11:19:05AM +0800, Lan Tianyu wrote:
> On 2012/10/25 5:49, Greg KH wrote:
> >On Wed, Oct 24, 2012 at 09:24:04PM +0800, Lan Tianyu wrote:
> >>于 2012/10/24 18:55, Sergei Shtylyov 写道:
> >>>>+ /* Create link files between child device and usb port device. */
> >>>>+ if (udev->parent) {
> >>>>+ int no_warn;
> >>>
> >>> I think 'ret' or 'result' is a better name...
> >>>
> >>>>+ struct usb_port *port_dev =
> >>>>+ hdev_to_hub(udev->parent)->ports[udev->portnum - 1];
> >>>>+
> >>>>+ no_warn = sysfs_create_link(&udev->dev.kobj,
> >>>>+ &port_dev->dev.kobj, "port");
> >>>>+ no_warn = sysfs_create_link(&port_dev->dev.kobj,
> >>>>+ &udev->dev.kobj, "child");
> >>>
> >>> I guess you are not supposed to ignore the result if the function requires
> >>>it not to be ignored.
> >>>
> >>Hi Sergei:
> >> Great thanks for your review. From my opinion, failure to create link will
> >>not affect usb device function and so the return value can be ignored, Perharps
> >>producing
> >>some warning will be better. Do you have some suggestion? :)
> >
> >Properly handle the error, don't ignore it.
> >
> HI Greg:
> Ok, I get it. How about following?
> + /* Create link files between child device and usb port device. */
> + if (udev->parent) {
> + struct usb_port *port_dev =
> + hdev_to_hub(udev->parent)->ports[udev->portnum - 1];
> +
> + err = sysfs_create_link(&udev->dev.kobj,
> + &port_dev->dev.kobj, "port");
> + if (err)
> + goto fail;
> +
> + err = sysfs_create_link(&port_dev->dev.kobj,
> + &udev->dev.kobj, "child");
> + if (err) {
> + sysfs_remove_link(&udev->dev.kobj, "port");
> + goto fail;
> + }
> + }
> +
It's a good start, did you test it out?
And why are you calling the devices in a port a "child"? It's a device.
Otherwise everything is a child, and that doesn't make any sense, right?
greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
