| 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
| ||
|
Message-ID: <Y+5Rb17FTG4IxcE0@smile.fi.intel.com>
Date: Thu, 16 Feb 2023 17:53:19 +0200
From: Andy Shevchenko <andriy.shevchenko@...el.com>
To: Tomi Valkeinen <tomi.valkeinen@...asonboard.com>
Cc: linux-media@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-i2c@...r.kernel.org,
Rob Herring <robh+dt@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
Wolfram Sang <wsa@...nel.org>,
Luca Ceresoli <luca.ceresoli@...tlin.com>,
Matti Vaittinen <Matti.Vaittinen@...rohmeurope.com>,
Laurent Pinchart <laurent.pinchart+renesas@...asonboard.com>,
Mauro Carvalho Chehab <mchehab@...nel.org>,
Peter Rosin <peda@...ntia.se>,
Liam Girdwood <lgirdwood@...il.com>,
Mark Brown <broonie@...nel.org>,
Sakari Ailus <sakari.ailus@...ux.intel.com>,
Michael Tretter <m.tretter@...gutronix.de>,
Shawn Tu <shawnx.tu@...el.com>,
Hans Verkuil <hverkuil@...all.nl>,
Mike Pagano <mpagano@...too.org>,
Krzysztof Hałasa <khalasa@...p.pl>,
Marek Vasut <marex@...x.de>,
Satish Nagireddy <satish.nagireddy@...cruise.com>
Subject: Re: [PATCH v9 0/8] i2c-atr and FPDLink
On Thu, Feb 16, 2023 at 04:07:39PM +0200, Tomi Valkeinen wrote:
...
> + if (!c2a)
I would expect here dev_warn() to let user know about "shouldn't happened, but
have happened" situation.
> + return; /* This shouldn't happen */
...
> - static const struct v4l2_mbus_framefmt format = {
> + static const struct v4l2_mbus_framefmt informat = {
Naming a bit confusing. Is it "information" that cut or what?
in_format
> + static const struct v4l2_mbus_framefmt outformat = {
out_format
...
> -out_unlock:
> +out:
Why?
...
> +/*
> + * (Possible) TODOs
TODOs:
> + *
> + * - PM for serializer and remote peripherals. We need to manage:
> + * - VPOC
> + * - Power domain? Regulator? Somehow any remote device should be able to
> + * cause the VPOC to be turned on.
> + * - Link between the deserializer and the serializer
> + * - Related to VPOC management. We probably always want to turn on the VPOC
> + * and then enable the link.
> + * - Serializer's services: i2c, gpios, power
> + * - The serializer needs to resume before the remote peripherals can
> + * e.g. use the i2c.
> + * - How to handle gpios? Reserving a gpio essentially keeps the provider
> + * (serializer) always powered on.
> + * - Do we need a new bus for the FPD-Link? At the moment the serializers
> + * are children of the same i2c-adapter where the deserializer resides.
> + * - i2c-atr could be made embeddable instead of allocatable.
> + */
...
> struct atr_alias_table_entry {
> u16 alias_id; /* Alias ID from DT */
>
> - bool reserved;
> + bool in_use;
> u8 nport;
> u8 slave_id; /* i2c client's local i2c address */
> u8 port_reg_idx;
Wouldn't be wiser to move boolean at the end so if any obscure
architecture/compiler makes it longer than a byte it won't increase the memory
footprint. (Actually wouldn't it be aligned to u16 followed by u8 as well as
they are different types?)
> };
...
> +static int ub960_read16(struct ub960_data *priv, u8 reg, u16 *val)
> +{
> + struct device *dev = &priv->client->dev;
> + unsigned int v1, v2;
> + int ret;
> +
> + mutex_lock(&priv->reg_lock);
> +
> + ret = regmap_read(priv->regmap, reg, &v1);
> + if (ret) {
> + dev_err(dev, "%s: cannot read register 0x%02x (%d)!\n",
> + __func__, reg, ret);
> + goto out_unlock;
> + }
> +
> + ret = regmap_read(priv->regmap, reg + 1, &v2);
> + if (ret) {
> + dev_err(dev, "%s: cannot read register 0x%02x (%d)!\n",
> + __func__, reg + 1, ret);
> + goto out_unlock;
> + }
Wondering why bulk read can't be used against properly typed __be16 variable?
> + *val = (v1 << 8) | v2;
+ be16_to_cpu() here.
> +out_unlock:
> + mutex_unlock(&priv->reg_lock);
> +
> + return ret;
> +}
...
> +static int ub960_rxport_read16(struct ub960_data *priv, u8 nport, u8 reg,
> + u16 *val)
> {
Ditto.
> +}
...
> struct i2c_board_info ser_info = {
> - .of_node = to_of_node(rxport->remote_fwnode),
> - .fwnode = rxport->remote_fwnode,
> + .of_node = to_of_node(rxport->ser.fwnode),
> + .fwnode = rxport->ser.fwnode,
Why do you need to have both?!
> .platform_data = ser_pdata,
> };
...
> + for (nport = 0; nport < priv->hw_data->num_rxports; ++nport) {
Pre-increment is non-standard in the kernel.
> + struct ub960_rxport *rxport = priv->rxports[nport];
> + struct v4l2_mbus_frame_desc desc;
> + int ret;
> + u8 cur_vc;
> +
> + if (!rxport)
> + continue;
> +
> + ret = v4l2_subdev_call(rxport->source.sd, pad, get_frame_desc,
> + rxport->source.pad, &desc);
> + if (ret)
> + return ret;
> +
> + if (desc.type != V4L2_MBUS_FRAME_DESC_TYPE_CSI2)
> + continue;
cur_vc = desc.entry[0].bus.csi2.vc;
> + for (i = 0; i < desc.num_entries; ++i) {
> + u8 vc = desc.entry[i].bus.csi2.vc;
> + if (i == 0) {
> + cur_vc = vc;
> + continue;
> + }
This is an invariant to the loop, see above.
> + if (vc == cur_vc)
> + continue;
> +
> + dev_err(&priv->client->dev,
> + "rx%u: source with multiple virtual-channels is not supported\n",
> + nport);
> + return -ENODEV;
> + }
> + }
...
> + for (i = 0; i < 6; ++i)
> ub960_read(priv, UB960_SR_FPD3_RX_ID(i), &id[i]);
> id[6] = 0;
Wondering if this magic can be defined.
...
> + priv->atr.aliases = devm_kcalloc(dev, table_size,
> + sizeof(struct atr_alias_table_entry),
sizeof(*priv->atr.aliases) ?
> + GFP_KERNEL);
> + if (!priv->atr.aliases)
> return -ENOMEM;
...
> if (ret) {
> if (ret != -EINVAL) {
> - dev_err(dev,
> - "rx%u: failed to read 'ti,strobe-pos': %d\n",
> - nport, ret);
> + dev_err(dev, "rx%u: failed to read '%s': %d\n", nport,
> + "ti,strobe-pos", ret);
> return ret;
> }
> } else if (strobe_pos < UB960_MIN_MANUAL_STROBE_POS ||
> @@ -3512,8 +3403,8 @@ ub960_parse_dt_rxport_link_properties(struct ub960_data *priv,
> ret = fwnode_property_read_u32(link_fwnode, "ti,eq-level", &eq_level);
> if (ret) {
> if (ret != -EINVAL) {
> - dev_err(dev, "rx%u: failed to read 'ti,eq-level': %d\n",
> - nport, ret);
> + dev_err(dev, "rx%u: failed to read '%s': %d\n", nport,
> + "ti,eq-level", ret);
> return ret;
> }
> } else if (eq_level > UB960_MAX_EQ_LEVEL) {
Seems like you may do (in both cases) similar to the above:
var = 0;
ret = read_u32();
if (ret && ret != -EINVAL) {
// error handling
}
if (var > limit) {
// another error handling
}
...
> + static const char *vpoc_names[UB960_MAX_RX_NPORTS] = { "vpoc0", "vpoc1",
> + "vpoc2", "vpoc3" };
Wouldn't be better to format it as
static const char *vpoc_names[UB960_MAX_RX_NPORTS] = {
"vpoc0", "vpoc1", "vpoc2", "vpoc3",
};
?
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists