lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20171227103055.GN3374@localhost>
Date:   Wed, 27 Dec 2017 11:30:55 +0100
From:   Johan Hovold <johan@...nel.org>
To:     "Ji-Ze Hong (Peter Hong)" <hpeter@...il.com>
Cc:     Johan Hovold <johan@...nel.org>, gregkh@...uxfoundation.org,
        linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org,
        "Ji-Ze Hong (Peter Hong)" <hpeter+linux_kernel@...il.com>
Subject: Re: [PATCH V1 3/4] usb: serial: f81534: add output pin control

On Thu, Dec 21, 2017 at 05:49:45PM +0800, Ji-Ze Hong (Peter Hong) wrote:
> Hi Johan,
> 
> Johan Hovold 於 2017/12/19 上午 12:06 寫道:
> > On Thu, Nov 16, 2017 at 03:46:08PM +0800, Ji-Ze Hong (Peter Hong) wrote:
> >> +static int f81534_set_port_output_pin(struct usb_serial_port *port)
> >> +{
> >> +	struct f81534_serial_private *serial_priv;
> >> +	struct f81534_port_private *port_priv;
> >> +	struct usb_serial *serial;
> >> +	const struct f81534_port_out_pin *pins;
> >> +	int status;
> >> +	int i;
> >> +	u8 value;
> >> +	u8 idx;
> >> +
> >> +	serial = port->serial;
> >> +	serial_priv = usb_get_serial_data(serial);
> >> +	port_priv = usb_get_serial_port_data(port);
> >> +
> >> +	idx = F81534_CONF_GPIO_OFFSET + port_priv->phy_num;
> >> +	value = serial_priv->conf_data[idx];
> >> +	pins = &f81534_port_out_pins[port_priv->phy_num];
> >> +
> >> +	for (i = 0; i < ARRAY_SIZE(pins->pin); ++i) {
> >> +		status = f81534_set_mask_register(serial,
> >> +				pins->pin[i].reg_addr, pins->pin[i].reg_mask,
> >> +				value & BIT(i) ? pins->pin[i].reg_mask : 0);
> >> +		if (status)
> >> +			return status;
> >> +	}
> > 
> > You're using 24 (get or set) accesses to update these three registers
> > here. Why not read them out (if necessary), determine their new values
> > and then write them back when done instead?
> > 
> 
> In this code, I'm only read/write 3 registers of 0x2ae8, 0x2a90, 0x2a80,
> but some register will read/write more than once. Should I change the
> code from port_probe() to attach() and re-write it as:
> 	1: read the 3 register
> 	2: change them will 12 pin desire value
> 	3: write it back
> Is it ok?

Do you expect these pins to ever be changed after probe? If not, then
perhaps it can be moved to attach(), but otherwise I guess they should
be set at port_probe(). By using shadow registers, you should be able to
reduce the number of device accesses, but perhaps it's not worth the
complexity.

Do you have a rough idea about how long these register updates take? I
was just worried that these changes will add up to really long probe
times.

Thanks,
Johan

Powered by blists - more mailing lists