| 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: <1379400c.90c.19582b05f74.Coremail.andyshrk@163.com>
Date: Tue, 11 Mar 2025 08:52:31 +0800 (CST)
From: "Andy Yan" <andyshrk@....com>
To: "Vinod Koul" <vkoul@...nel.org>
Cc: heiko@...ech.de, kishon@...nel.org, sebastian.reichel@...labora.com,
yubing.zhang@...k-chips.com, dmitry.baryshkov@...aro.org,
frank.wang@...k-chips.com, linux-arm-kernel@...ts.infradead.org,
linux-kernel@...r.kernel.org, linux-phy@...ts.infradead.org,
linux-rockchip@...ts.infradead.org,
"Andy Yan" <andy.yan@...k-chips.com>
Subject: Re:Re: [PATCH] phy: rockchip: usbdp: Check these parameters only
when the corresponding set flags are set
Hi Vinod,
At 2025-03-10 15:26:50, "Vinod Koul" <vkoul@...nel.org> wrote:
>On 06-03-25, 14:59, Andy Yan wrote:
>> From: Andy Yan <andy.yan@...k-chips.com>
>>
>> According documentation of phy_configure_opts_dp, at the configure
>> stage, we should only verify/configure the link_rate when set_rate
>> flag is set, the same applies to lanes and voltage.
>>
>> So we do it as the documentation says, also record the link rate
>> and lanes in phy internal for set_voltate stage.
>
>Whenever you say also, that is a sign that it should be another patch!
This patch is inspired by Dmitry's suggestion for DP/eDP driver which calls phy_configure[0][1]:
the phy driver should skip the values for which the .set_foo isn't set. So we should not set rates and
lanes when we set_voltate, but the rates and lanes are needed for voltage set, this is why I record
rates and lanes in this patch.
I can split the record to another patch, but then this patch will not work correctly itself,
I'm not sure if this is suitable.
[0]https://patchwork.freedesktop.org/patch/629780/?series=141829&rev=3
[1]https://patchwork.freedesktop.org/patch/638961/?series=145286&rev=1
>
>>
>> Signed-off-by: Andy Yan <andy.yan@...k-chips.com>
>> ---
>>
>> drivers/phy/rockchip/phy-rockchip-usbdp.c | 63 +++++++++++------------
>> 1 file changed, 31 insertions(+), 32 deletions(-)
>>
>> diff --git a/drivers/phy/rockchip/phy-rockchip-usbdp.c b/drivers/phy/rockchip/phy-rockchip-usbdp.c
>> index c04cf64f8a35..d1bbdf382aa2 100644
>> --- a/drivers/phy/rockchip/phy-rockchip-usbdp.c
>> +++ b/drivers/phy/rockchip/phy-rockchip-usbdp.c
>> @@ -187,6 +187,8 @@ struct rk_udphy {
>> u32 dp_aux_din_sel;
>> bool dp_sink_hpd_sel;
>> bool dp_sink_hpd_cfg;
>> + unsigned int link_rate;
>> + unsigned int lanes;
>> u8 bw;
>> int id;
>>
>> @@ -1102,42 +1104,39 @@ static int rk_udphy_dp_phy_power_off(struct phy *phy)
>> return 0;
>> }
>>
>> -static int rk_udphy_dp_phy_verify_link_rate(unsigned int link_rate)
>> -{
>> - switch (link_rate) {
>> - case 1620:
>> - case 2700:
>> - case 5400:
>> - case 8100:
>> - break;
>> -
>> - default:
>> - return -EINVAL;
>> - }
>> -
>> - return 0;
>> -}
>> -
>> static int rk_udphy_dp_phy_verify_config(struct rk_udphy *udphy,
>> struct phy_configure_opts_dp *dp)
>> {
>> - int i, ret;
>> + int i;
>>
>> - /* If changing link rate was required, verify it's supported. */
>> - ret = rk_udphy_dp_phy_verify_link_rate(dp->link_rate);
>> - if (ret)
>> - return ret;
>> + /* Verify link rate. */
>> + if (dp->set_rate) {
>> + switch (dp->link_rate) {
>> + case 1620:
>> + case 2700:
>> + case 5400:
>> + case 8100:
>> + udphy->link_rate = dp->link_rate;
>> + break;
>> +
>> + default:
>> + return -EINVAL;
>> + }
>
>why drop helper? Why not set the rate on success?
I drop helper just because the flowing check for lanes and votages don't
have helper.
The rate is set in rk_udphy_dp_phy_configure.
>
>> + }
>>
>> /* Verify lane count. */
>> - switch (dp->lanes) {
>> - case 1:
>> - case 2:
>> - case 4:
>> - /* valid lane count. */
>> - break;
>> + if (dp->set_lanes) {
>> + switch (dp->lanes) {
>> + case 1:
>> + case 2:
>> + case 4:
>> + /* valid lane count. */
>> + udphy->lanes = dp->lanes;
>> + break;
>>
>> - default:
>> - return -EINVAL;
>> + default:
>> + return -EINVAL;
>> + }
>
>another change where helper would have made this look better
Do you mean we should do it like this:
static int rk_udphy_dp_phy_configure(struct phy *phy,
union phy_configure_opts *opts)
{
struct rk_udphy *udphy = phy_get_drvdata(phy);
struct phy_configure_opts_dp *dp = &opts->dp;
u32 i, val, lane;
int ret;
.............
if (dp->set_rate)
ret = rk_udphy_dp_phy_verify_link_rate(dp->link_rate);
if (dp->set_lanes)
ret = rk_udphy_dp_phy_verify_link_lanes(dp->lanes);
if (dp->set_voltates)
ret = rk_udphy_dp_phy_verify_link_voltate(dp->voltate);
Add helper for each of them ?
>
>> }
>>
>> /*
>> @@ -1146,7 +1145,7 @@ static int rk_udphy_dp_phy_verify_config(struct rk_udphy *udphy,
>> */
>> if (dp->set_voltages) {
>> /* Lane count verified previously. */
>> - for (i = 0; i < dp->lanes; i++) {
>> + for (i = 0; i < udphy->lanes; i++) {
>> if (dp->voltage[i] > 3 || dp->pre[i] > 3)
>> return -EINVAL;
>>
>> @@ -1243,9 +1242,9 @@ static int rk_udphy_dp_phy_configure(struct phy *phy,
>> }
>>
>> if (dp->set_voltages) {
>> - for (i = 0; i < dp->lanes; i++) {
>> + for (i = 0; i < udphy->lanes; i++) {
>> lane = udphy->dp_lane_sel[i];
>> - switch (dp->link_rate) {
>> + switch (udphy->link_rate) {
>> case 1620:
>> case 2700:
>> regmap_update_bits(udphy->pma_regmap,
>> --
>> 2.34.1
>
>--
>~Vinod
>
>_______________________________________________
>Linux-rockchip mailing list
>Linux-rockchip@...ts.infradead.org
>http://lists.infradead.org/mailman/listinfo/linux-rockchip
Powered by blists - more mailing lists