LinuxLists.cc - [PATCH 0/9] Loading optional firmware v3

[permalink] [raw]

Subject: [PATCH 6/9] firmware: print firmware name on fallback path

Previously, one could assume the firmware name from the preceding
message: "Direct firmware load for {name} failed with error %d".

However, with the new firmware_request_nowarn() entrypoint, the message
outlined above will not always be printed.

Therefore, we add the firmware name to the fallback path spew in order
to associate it with the appropriate request.

Signed-off-by: Andres Rodriguez <[email protected]>
---
drivers/base/firmware_loader/fallback.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/base/firmware_loader/fallback.c b/drivers/base/firmware_loader/fallback.c
index e75928458489..1a47ddc70c31 100644
--- a/drivers/base/firmware_loader/fallback.c
+++ b/drivers/base/firmware_loader/fallback.c
@@ -669,6 +669,6 @@ int fw_sysfs_fallback(struct firmware *fw, const char *name,
if (!fw_run_sysfs_fallback(opt_flags))
return ret;

- dev_warn(device, "Falling back to user helper\n");
+ dev_warn(device, "Falling back to user helper for %s\n", name);
return fw_load_from_user_helper(fw, name, device, opt_flags);
}
--
2.14.1

2018-04-17 15:35:08

[permalink] [raw]

Subject: [PATCH 8/9] ath10k: use request_firmware_nowarn to load firmware

2018-04-17 15:35:27

[permalink] [raw]

Subject: [PATCH 9/9] brcmfmac: use request_firmware_nowait2 to load firmware without warnings

This reduces the unnecessary spew when trying to load optional firmware:
"Direct firmware load for ... failed with error -2"

Signed-off-by: Andres Rodriguez <[email protected]>
---
drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
index 091b52979e03..26db3ebd52dc 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
@@ -503,8 +503,9 @@ static void brcmf_fw_request_code_done(const struct firmware *fw, void *ctx)
goto done;

fwctx->code = fw;
- ret = request_firmware_nowait(THIS_MODULE, true, fwctx->nvram_name,
- fwctx->dev, GFP_KERNEL, fwctx,
+ ret = request_firmware_nowait(THIS_MODULE, true, false,
+ fwctx->nvram_name, fwctx->dev,
+ GFP_KERNEL, fwctx,
brcmf_fw_request_nvram_done);

/* pass NULL to nvram callback for bcm47xx fallback */
@@ -547,7 +548,7 @@ int brcmf_fw_get_firmwares_pcie(struct device *dev, u16 flags,
fwctx->domain_nr = domain_nr;
fwctx->bus_nr = bus_nr;

- return request_firmware_nowait(THIS_MODULE, true, code, dev,
+ return request_firmware_nowait2(THIS_MODULE, true, false, code, dev,
GFP_KERNEL, fwctx,
brcmf_fw_request_code_done);
}
--
2.14.1

2018-04-17 15:36:24

[permalink] [raw]

Subject: [PATCH 7/9] drm/amdgpu: use firmware_request_nowarn to load firmware

Currently, during the normal boot process the amdgpu driver will produce
spew like the following in dmesg:
Direct firmware load for amdgpu/polaris10_mec_2.bin failed with error -2

This happens when amdgpu tries to load optional firmware files. So the
error does not affect the startup sequence.

This patch switches the amdgpu to use firmware_request_nowarn(), which
will not produce the warnings mentioned above. Hopefully resulting in a
cleaner bootup log.

Signed-off-by: Andres Rodriguez <[email protected]>
---
drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c | 2 +-
drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 4 ++--
drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.h | 2 +-
drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c | 2 +-
drivers/gpu/drm/amd/amdgpu/amdgpu_vce.c | 2 +-
drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c | 2 +-
drivers/gpu/drm/amd/amdgpu/ci_dpm.c | 2 +-
drivers/gpu/drm/amd/amdgpu/cik_sdma.c | 2 +-
drivers/gpu/drm/amd/amdgpu/gfx_v6_0.c | 8 ++++----
drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c | 12 +++++------
drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c | 32 +++++++++++++++---------------
drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c | 12 +++++------
drivers/gpu/drm/amd/amdgpu/gmc_v6_0.c | 2 +-
drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c | 2 +-
drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c | 2 +-
drivers/gpu/drm/amd/amdgpu/psp_v10_0.c | 2 +-
drivers/gpu/drm/amd/amdgpu/psp_v3_1.c | 4 ++--
drivers/gpu/drm/amd/amdgpu/sdma_v2_4.c | 2 +-
drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c | 2 +-
drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c | 2 +-
drivers/gpu/drm/amd/amdgpu/si_dpm.c | 2 +-
21 files changed, 51 insertions(+), 51 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c
index 4466f3535e2d..6c950811c0a5 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c
@@ -811,7 +811,7 @@ static int amdgpu_cgs_get_firmware_info(struct cgs_device *cgs_device,
return -EINVAL;
}

- err = request_firmware(&adev->pm.fw, fw_name, adev->dev);
+ err = firmware_request_nowarn(&adev->pm.fw, fw_name, adev->dev);
if (err) {
DRM_ERROR("Failed to request firmware\n");
return err;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index af1b879a9ee9..d6225619e69f 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -696,7 +696,7 @@ bool amdgpu_device_need_post(struct amdgpu_device *adev)
if (adev->asic_type == CHIP_FIJI) {
int err;
uint32_t fw_ver;
- err = request_firmware(&adev->pm.fw, "amdgpu/fiji_smc.bin", adev->dev);
+ err = firmware_request_nowarn(&adev->pm.fw, "amdgpu/fiji_smc.bin", adev->dev);
/* force vPost if error occured */
if (err)
return true;
@@ -1133,7 +1133,7 @@ static int amdgpu_device_parse_gpu_info_fw(struct amdgpu_device *adev)
}

snprintf(fw_name, sizeof(fw_name), "amdgpu/%s_gpu_info.bin", chip_name);
- err = request_firmware(&adev->firmware.gpu_info_fw, fw_name, adev->dev);
+ err = firmware_request_nowarn(&adev->firmware.gpu_info_fw, fw_name, adev->dev);
if (err) {
dev_err(adev->dev,
"Failed to load gpu_info firmware \"%s\"\n",
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.h
index 30b5500dc152..0acd1f3d14c8 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.h
@@ -196,7 +196,7 @@ enum AMDGPU_UCODE_STATUS {
struct amdgpu_firmware_info {
/* ucode ID */
enum AMDGPU_UCODE_ID ucode_id;
- /* request_firmware */
+ /* firmware_request */
const struct firmware *fw;
/* starting mc address */
uint64_t mc_addr;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c
index b2eae86bf906..4de018d45081 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c
@@ -171,7 +171,7 @@ int amdgpu_uvd_sw_init(struct amdgpu_device *adev)
return -EINVAL;
}

- r = request_firmware(&adev->uvd.fw, fw_name, adev->dev);
+ r = firmware_request_nowarn(&adev->uvd.fw, fw_name, adev->dev);
if (r) {
dev_err(adev->dev, "amdgpu_uvd: Can't load firmware \"%s\"\n",
fw_name);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vce.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vce.c
index d274ae535530..b6af824a2f44 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vce.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vce.c
@@ -138,7 +138,7 @@ int amdgpu_vce_sw_init(struct amdgpu_device *adev, unsigned long size)
return -EINVAL;
}

- r = request_firmware(&adev->vce.fw, fw_name, adev->dev);
+ r = firmware_request_nowarn(&adev->vce.fw, fw_name, adev->dev);
if (r) {
dev_err(adev->dev, "amdgpu_vce: Can't load firmware \"%s\"\n",
fw_name);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
index 837962118dbc..bd650b87e281 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
@@ -67,7 +67,7 @@ int amdgpu_vcn_sw_init(struct amdgpu_device *adev)
return -EINVAL;
}

- r = request_firmware(&adev->vcn.fw, fw_name, adev->dev);
+ r = firmware_request_nowarn(&adev->vcn.fw, fw_name, adev->dev);
if (r) {
dev_err(adev->dev, "amdgpu_vcn: Can't load firmware \"%s\"\n",
fw_name);
diff --git a/drivers/gpu/drm/amd/amdgpu/ci_dpm.c b/drivers/gpu/drm/amd/amdgpu/ci_dpm.c
index a0943aa8d1d3..95e1edc1311d 100644
--- a/drivers/gpu/drm/amd/amdgpu/ci_dpm.c
+++ b/drivers/gpu/drm/amd/amdgpu/ci_dpm.c
@@ -5849,7 +5849,7 @@ static int ci_dpm_init_microcode(struct amdgpu_device *adev)
}

snprintf(fw_name, sizeof(fw_name), "radeon/%s_smc.bin", chip_name);
- err = request_firmware(&adev->pm.fw, fw_name, adev->dev);
+ err = firmware_request_nowarn(&adev->pm.fw, fw_name, adev->dev);
if (err)
goto out;
err = amdgpu_ucode_validate(adev->pm.fw);
diff --git a/drivers/gpu/drm/amd/amdgpu/cik_sdma.c b/drivers/gpu/drm/amd/amdgpu/cik_sdma.c
index 6e8278e689b1..93c8acca0360 100644
--- a/drivers/gpu/drm/amd/amdgpu/cik_sdma.c
+++ b/drivers/gpu/drm/amd/amdgpu/cik_sdma.c
@@ -135,7 +135,7 @@ static int cik_sdma_init_microcode(struct amdgpu_device *adev)
snprintf(fw_name, sizeof(fw_name), "radeon/%s_sdma.bin", chip_name);
else
snprintf(fw_name, sizeof(fw_name), "radeon/%s_sdma1.bin", chip_name);
- err = request_firmware(&adev->sdma.instance[i].fw, fw_name, adev->dev);
+ err = firmware_request_nowarn(&adev->sdma.instance[i].fw, fw_name, adev->dev);
if (err)
goto out;
err = amdgpu_ucode_validate(adev->sdma.instance[i].fw);
diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v6_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v6_0.c
index 9870d83b68c1..8aebab5edf15 100644
--- a/drivers/gpu/drm/amd/amdgpu/gfx_v6_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gfx_v6_0.c
@@ -335,7 +335,7 @@ static int gfx_v6_0_init_microcode(struct amdgpu_device *adev)
}

snprintf(fw_name, sizeof(fw_name), "radeon/%s_pfp.bin", chip_name);
- err = request_firmware(&adev->gfx.pfp_fw, fw_name, adev->dev);
+ err = firmware_request_nowarn(&adev->gfx.pfp_fw, fw_name, adev->dev);
if (err)
goto out;
err = amdgpu_ucode_validate(adev->gfx.pfp_fw);
@@ -346,7 +346,7 @@ static int gfx_v6_0_init_microcode(struct amdgpu_device *adev)
adev->gfx.pfp_feature_version = le32_to_cpu(cp_hdr->ucode_feature_version);

snprintf(fw_name, sizeof(fw_name), "radeon/%s_me.bin", chip_name);
- err = request_firmware(&adev->gfx.me_fw, fw_name, adev->dev);
+ err = firmware_request_nowarn(&adev->gfx.me_fw, fw_name, adev->dev);
if (err)
goto out;
err = amdgpu_ucode_validate(adev->gfx.me_fw);
@@ -357,7 +357,7 @@ static int gfx_v6_0_init_microcode(struct amdgpu_device *adev)
adev->gfx.me_feature_version = le32_to_cpu(cp_hdr->ucode_feature_version);

snprintf(fw_name, sizeof(fw_name), "radeon/%s_ce.bin", chip_name);
- err = request_firmware(&adev->gfx.ce_fw, fw_name, adev->dev);
+ err = firmware_request_nowarn(&adev->gfx.ce_fw, fw_name, adev->dev);
if (err)
goto out;
err = amdgpu_ucode_validate(adev->gfx.ce_fw);
@@ -368,7 +368,7 @@ static int gfx_v6_0_init_microcode(struct amdgpu_device *adev)
adev->gfx.ce_feature_version = le32_to_cpu(cp_hdr->ucode_feature_version);

snprintf(fw_name, sizeof(fw_name), "radeon/%s_rlc.bin", chip_name);
- err = request_firmware(&adev->gfx.rlc_fw, fw_name, adev->dev);
+ err = firmware_request_nowarn(&adev->gfx.rlc_fw, fw_name, adev->dev);
if (err)
goto out;
err = amdgpu_ucode_validate(adev->gfx.rlc_fw);
diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
index a066c5eda135..35a0e46464a5 100644
--- a/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
@@ -926,7 +926,7 @@ static int gfx_v7_0_init_microcode(struct amdgpu_device *adev)
}

snprintf(fw_name, sizeof(fw_name), "radeon/%s_pfp.bin", chip_name);
- err = request_firmware(&adev->gfx.pfp_fw, fw_name, adev->dev);
+ err = firmware_request_nowarn(&adev->gfx.pfp_fw, fw_name, adev->dev);
if (err)
goto out;
err = amdgpu_ucode_validate(adev->gfx.pfp_fw);
@@ -934,7 +934,7 @@ static int gfx_v7_0_init_microcode(struct amdgpu_device *adev)
goto out;

snprintf(fw_name, sizeof(fw_name), "radeon/%s_me.bin", chip_name);
- err = request_firmware(&adev->gfx.me_fw, fw_name, adev->dev);
+ err = firmware_request_nowarn(&adev->gfx.me_fw, fw_name, adev->dev);
if (err)
goto out;
err = amdgpu_ucode_validate(adev->gfx.me_fw);
@@ -942,7 +942,7 @@ static int gfx_v7_0_init_microcode(struct amdgpu_device *adev)
goto out;

snprintf(fw_name, sizeof(fw_name), "radeon/%s_ce.bin", chip_name);
- err = request_firmware(&adev->gfx.ce_fw, fw_name, adev->dev);
+ err = firmware_request_nowarn(&adev->gfx.ce_fw, fw_name, adev->dev);
if (err)
goto out;
err = amdgpu_ucode_validate(adev->gfx.ce_fw);
@@ -950,7 +950,7 @@ static int gfx_v7_0_init_microcode(struct amdgpu_device *adev)
goto out;

snprintf(fw_name, sizeof(fw_name), "radeon/%s_mec.bin", chip_name);
- err = request_firmware(&adev->gfx.mec_fw, fw_name, adev->dev);
+ err = firmware_request_nowarn(&adev->gfx.mec_fw, fw_name, adev->dev);
if (err)
goto out;
err = amdgpu_ucode_validate(adev->gfx.mec_fw);
@@ -959,7 +959,7 @@ static int gfx_v7_0_init_microcode(struct amdgpu_device *adev)

if (adev->asic_type == CHIP_KAVERI) {
snprintf(fw_name, sizeof(fw_name), "radeon/%s_mec2.bin", chip_name);
- err = request_firmware(&adev->gfx.mec2_fw, fw_name, adev->dev);
+ err = firmware_request_nowarn(&adev->gfx.mec2_fw, fw_name, adev->dev);
if (err)
goto out;
err = amdgpu_ucode_validate(adev->gfx.mec2_fw);
@@ -968,7 +968,7 @@ static int gfx_v7_0_init_microcode(struct amdgpu_device *adev)
}

snprintf(fw_name, sizeof(fw_name), "radeon/%s_rlc.bin", chip_name);
- err = request_firmware(&adev->gfx.rlc_fw, fw_name, adev->dev);
+ err = firmware_request_nowarn(&adev->gfx.rlc_fw, fw_name, adev->dev);
if (err)
goto out;
err = amdgpu_ucode_validate(adev->gfx.rlc_fw);
diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
index 4e694ae9f308..c16cd96a1557 100644
--- a/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
@@ -936,14 +936,14 @@ static int gfx_v8_0_init_microcode(struct amdgpu_device *adev)

if (adev->asic_type >= CHIP_POLARIS10 && adev->asic_type <= CHIP_POLARIS12) {
snprintf(fw_name, sizeof(fw_name), "amdgpu/%s_pfp_2.bin", chip_name);
- err = request_firmware(&adev->gfx.pfp_fw, fw_name, adev->dev);
+ err = firmware_request_nowarn(&adev->gfx.pfp_fw, fw_name, adev->dev);
if (err == -ENOENT) {
snprintf(fw_name, sizeof(fw_name), "amdgpu/%s_pfp.bin", chip_name);
- err = request_firmware(&adev->gfx.pfp_fw, fw_name, adev->dev);
+ err = firmware_request_nowarn(&adev->gfx.pfp_fw, fw_name, adev->dev);
}
} else {
snprintf(fw_name, sizeof(fw_name), "amdgpu/%s_pfp.bin", chip_name);
- err = request_firmware(&adev->gfx.pfp_fw, fw_name, adev->dev);
+ err = firmware_request_nowarn(&adev->gfx.pfp_fw, fw_name, adev->dev);
}
if (err)
goto out;
@@ -956,14 +956,14 @@ static int gfx_v8_0_init_microcode(struct amdgpu_device *adev)

if (adev->asic_type >= CHIP_POLARIS10 && adev->asic_type <= CHIP_POLARIS12) {
snprintf(fw_name, sizeof(fw_name), "amdgpu/%s_me_2.bin", chip_name);
- err = request_firmware(&adev->gfx.me_fw, fw_name, adev->dev);
+ err = firmware_request_nowarn(&adev->gfx.me_fw, fw_name, adev->dev);
if (err == -ENOENT) {
snprintf(fw_name, sizeof(fw_name), "amdgpu/%s_me.bin", chip_name);
- err = request_firmware(&adev->gfx.me_fw, fw_name, adev->dev);
+ err = firmware_request_nowarn(&adev->gfx.me_fw, fw_name, adev->dev);
}
} else {
snprintf(fw_name, sizeof(fw_name), "amdgpu/%s_me.bin", chip_name);
- err = request_firmware(&adev->gfx.me_fw, fw_name, adev->dev);
+ err = firmware_request_nowarn(&adev->gfx.me_fw, fw_name, adev->dev);
}
if (err)
goto out;
@@ -977,14 +977,14 @@ static int gfx_v8_0_init_microcode(struct amdgpu_device *adev)

if (adev->asic_type >= CHIP_POLARIS10 && adev->asic_type <= CHIP_POLARIS12) {
snprintf(fw_name, sizeof(fw_name), "amdgpu/%s_ce_2.bin", chip_name);
- err = request_firmware(&adev->gfx.ce_fw, fw_name, adev->dev);
+ err = firmware_request_nowarn(&adev->gfx.ce_fw, fw_name, adev->dev);
if (err == -ENOENT) {
snprintf(fw_name, sizeof(fw_name), "amdgpu/%s_ce.bin", chip_name);
- err = request_firmware(&adev->gfx.ce_fw, fw_name, adev->dev);
+ err = firmware_request_nowarn(&adev->gfx.ce_fw, fw_name, adev->dev);
}
} else {
snprintf(fw_name, sizeof(fw_name), "amdgpu/%s_ce.bin", chip_name);
- err = request_firmware(&adev->gfx.ce_fw, fw_name, adev->dev);
+ err = firmware_request_nowarn(&adev->gfx.ce_fw, fw_name, adev->dev);
}
if (err)
goto out;
@@ -1007,7 +1007,7 @@ static int gfx_v8_0_init_microcode(struct amdgpu_device *adev)
adev->virt.chained_ib_support = false;

snprintf(fw_name, sizeof(fw_name), "amdgpu/%s_rlc.bin", chip_name);
- err = request_firmware(&adev->gfx.rlc_fw, fw_name, adev->dev);
+ err = firmware_request_nowarn(&adev->gfx.rlc_fw, fw_name, adev->dev);
if (err)
goto out;
err = amdgpu_ucode_validate(adev->gfx.rlc_fw);
@@ -1057,14 +1057,14 @@ static int gfx_v8_0_init_microcode(struct amdgpu_device *adev)

if (adev->asic_type >= CHIP_POLARIS10 && adev->asic_type <= CHIP_POLARIS12) {
snprintf(fw_name, sizeof(fw_name), "amdgpu/%s_mec_2.bin", chip_name);
- err = request_firmware(&adev->gfx.mec_fw, fw_name, adev->dev);
+ err = firmware_request_nowarn(&adev->gfx.mec_fw, fw_name, adev->dev);
if (err == -ENOENT) {
snprintf(fw_name, sizeof(fw_name), "amdgpu/%s_mec.bin", chip_name);
- err = request_firmware(&adev->gfx.mec_fw, fw_name, adev->dev);
+ err = firmware_request_nowarn(&adev->gfx.mec_fw, fw_name, adev->dev);
}
} else {
snprintf(fw_name, sizeof(fw_name), "amdgpu/%s_mec.bin", chip_name);
- err = request_firmware(&adev->gfx.mec_fw, fw_name, adev->dev);
+ err = firmware_request_nowarn(&adev->gfx.mec_fw, fw_name, adev->dev);
}
if (err)
goto out;
@@ -1079,14 +1079,14 @@ static int gfx_v8_0_init_microcode(struct amdgpu_device *adev)
(adev->asic_type != CHIP_TOPAZ)) {
if (adev->asic_type >= CHIP_POLARIS10 && adev->asic_type <= CHIP_POLARIS12) {
snprintf(fw_name, sizeof(fw_name), "amdgpu/%s_mec2_2.bin", chip_name);
- err = request_firmware(&adev->gfx.mec2_fw, fw_name, adev->dev);
+ err = firmware_request_nowarn(&adev->gfx.mec2_fw, fw_name, adev->dev);
if (err == -ENOENT) {
snprintf(fw_name, sizeof(fw_name), "amdgpu/%s_mec2.bin", chip_name);
- err = request_firmware(&adev->gfx.mec2_fw, fw_name, adev->dev);
+ err = firmware_request_nowarn(&adev->gfx.mec2_fw, fw_name, adev->dev);
}
} else {
snprintf(fw_name, sizeof(fw_name), "amdgpu/%s_mec2.bin", chip_name);
- err = request_firmware(&adev->gfx.mec2_fw, fw_name, adev->dev);
+ err = firmware_request_nowarn(&adev->gfx.mec2_fw, fw_name, adev->dev);
}
if (!err) {
err = amdgpu_ucode_validate(adev->gfx.mec2_fw);
diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
index c06479615e8a..9f70012c81ad 100644
--- a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
@@ -370,7 +370,7 @@ static int gfx_v9_0_init_microcode(struct amdgpu_device *adev)
}

snprintf(fw_name, sizeof(fw_name), "amdgpu/%s_pfp.bin", chip_name);
- err = request_firmware(&adev->gfx.pfp_fw, fw_name, adev->dev);
+ err = firmware_request_nowarn(&adev->gfx.pfp_fw, fw_name, adev->dev);
if (err)
goto out;
err = amdgpu_ucode_validate(adev->gfx.pfp_fw);
@@ -381,7 +381,7 @@ static int gfx_v9_0_init_microcode(struct amdgpu_device *adev)
adev->gfx.pfp_feature_version = le32_to_cpu(cp_hdr->ucode_feature_version);

snprintf(fw_name, sizeof(fw_name), "amdgpu/%s_me.bin", chip_name);
- err = request_firmware(&adev->gfx.me_fw, fw_name, adev->dev);
+ err = firmware_request_nowarn(&adev->gfx.me_fw, fw_name, adev->dev);
if (err)
goto out;
err = amdgpu_ucode_validate(adev->gfx.me_fw);
@@ -392,7 +392,7 @@ static int gfx_v9_0_init_microcode(struct amdgpu_device *adev)
adev->gfx.me_feature_version = le32_to_cpu(cp_hdr->ucode_feature_version);

snprintf(fw_name, sizeof(fw_name), "amdgpu/%s_ce.bin", chip_name);
- err = request_firmware(&adev->gfx.ce_fw, fw_name, adev->dev);
+ err = firmware_request_nowarn(&adev->gfx.ce_fw, fw_name, adev->dev);
if (err)
goto out;
err = amdgpu_ucode_validate(adev->gfx.ce_fw);
@@ -403,7 +403,7 @@ static int gfx_v9_0_init_microcode(struct amdgpu_device *adev)
adev->gfx.ce_feature_version = le32_to_cpu(cp_hdr->ucode_feature_version);

snprintf(fw_name, sizeof(fw_name), "amdgpu/%s_rlc.bin", chip_name);
- err = request_firmware(&adev->gfx.rlc_fw, fw_name, adev->dev);
+ err = firmware_request_nowarn(&adev->gfx.rlc_fw, fw_name, adev->dev);
if (err)
goto out;
err = amdgpu_ucode_validate(adev->gfx.rlc_fw);
@@ -449,7 +449,7 @@ static int gfx_v9_0_init_microcode(struct amdgpu_device *adev)
adev->gfx.rlc.register_restore[i] = le32_to_cpu(tmp[i]);

snprintf(fw_name, sizeof(fw_name), "amdgpu/%s_mec.bin", chip_name);
- err = request_firmware(&adev->gfx.mec_fw, fw_name, adev->dev);
+ err = firmware_request_nowarn(&adev->gfx.mec_fw, fw_name, adev->dev);
if (err)
goto out;
err = amdgpu_ucode_validate(adev->gfx.mec_fw);
@@ -461,7 +461,7 @@ static int gfx_v9_0_init_microcode(struct amdgpu_device *adev)

snprintf(fw_name, sizeof(fw_name), "amdgpu/%s_mec2.bin", chip_name);
- err = request_firmware(&adev->gfx.mec2_fw, fw_name, adev->dev);
+ err = firmware_request_nowarn(&adev->gfx.mec2_fw, fw_name, adev->dev);
if (!err) {
err = amdgpu_ucode_validate(adev->gfx.mec2_fw);
if (err)
diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v6_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v6_0.c
index 8e28270d1ea9..4192a5a0c444 100644
--- a/drivers/gpu/drm/amd/amdgpu/gmc_v6_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gmc_v6_0.c
@@ -136,7 +136,7 @@ static int gmc_v6_0_init_microcode(struct amdgpu_device *adev)
snprintf(fw_name, sizeof(fw_name), "radeon/si58_mc.bin");
else
snprintf(fw_name, sizeof(fw_name), "radeon/%s_mc.bin", chip_name);
- err = request_firmware(&adev->mc.fw, fw_name, adev->dev);
+ err = firmware_request_nowarn(&adev->mc.fw, fw_name, adev->dev);
if (err)
goto out;

diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c
index 86e9d682c59e..06deba7f707d 100644
--- a/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c
@@ -151,7 +151,7 @@ static int gmc_v7_0_init_microcode(struct amdgpu_device *adev)
else
snprintf(fw_name, sizeof(fw_name), "radeon/%s_mc.bin", chip_name);

- err = request_firmware(&adev->mc.fw, fw_name, adev->dev);
+ err = firmware_request_nowarn(&adev->mc.fw, fw_name, adev->dev);
if (err)
goto out;
err = amdgpu_ucode_validate(adev->mc.fw);
diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
index 9a813d834f1a..cbce96198dbc 100644
--- a/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
@@ -235,7 +235,7 @@ static int gmc_v8_0_init_microcode(struct amdgpu_device *adev)
}

snprintf(fw_name, sizeof(fw_name), "amdgpu/%s_mc.bin", chip_name);
- err = request_firmware(&adev->mc.fw, fw_name, adev->dev);
+ err = firmware_request_nowarn(&adev->mc.fw, fw_name, adev->dev);
if (err)
goto out;
err = amdgpu_ucode_validate(adev->mc.fw);
diff --git a/drivers/gpu/drm/amd/amdgpu/psp_v10_0.c b/drivers/gpu/drm/amd/amdgpu/psp_v10_0.c
index 5a9fe24697f9..718722ef1835 100644
--- a/drivers/gpu/drm/amd/amdgpu/psp_v10_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/psp_v10_0.c
@@ -105,7 +105,7 @@ int psp_v10_0_init_microcode(struct psp_context *psp)
}

snprintf(fw_name, sizeof(fw_name), "amdgpu/%s_asd.bin", chip_name);
- err = request_firmware(&adev->psp.asd_fw, fw_name, adev->dev);
+ err = firmware_request_nowarn(&adev->psp.asd_fw, fw_name, adev->dev);
if (err)
goto out;

diff --git a/drivers/gpu/drm/amd/amdgpu/psp_v3_1.c b/drivers/gpu/drm/amd/amdgpu/psp_v3_1.c
index 19bd1934e63d..dd5261577d9b 100644
--- a/drivers/gpu/drm/amd/amdgpu/psp_v3_1.c
+++ b/drivers/gpu/drm/amd/amdgpu/psp_v3_1.c
@@ -111,7 +111,7 @@ int psp_v3_1_init_microcode(struct psp_context *psp)
}

snprintf(fw_name, sizeof(fw_name), "amdgpu/%s_sos.bin", chip_name);
- err = request_firmware(&adev->psp.sos_fw, fw_name, adev->dev);
+ err = firmware_request_nowarn(&adev->psp.sos_fw, fw_name, adev->dev);
if (err)
goto out;

@@ -131,7 +131,7 @@ int psp_v3_1_init_microcode(struct psp_context *psp)
le32_to_cpu(hdr->sos_offset_bytes);

snprintf(fw_name, sizeof(fw_name), "amdgpu/%s_asd.bin", chip_name);
- err = request_firmware(&adev->psp.asd_fw, fw_name, adev->dev);
+ err = firmware_request_nowarn(&adev->psp.asd_fw, fw_name, adev->dev);
if (err)
goto out;

diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v2_4.c b/drivers/gpu/drm/amd/amdgpu/sdma_v2_4.c
index d4787ad4d346..a2afbaacc7e5 100644
--- a/drivers/gpu/drm/amd/amdgpu/sdma_v2_4.c
+++ b/drivers/gpu/drm/amd/amdgpu/sdma_v2_4.c
@@ -146,7 +146,7 @@ static int sdma_v2_4_init_microcode(struct amdgpu_device *adev)
snprintf(fw_name, sizeof(fw_name), "amdgpu/%s_sdma.bin", chip_name);
else
snprintf(fw_name, sizeof(fw_name), "amdgpu/%s_sdma1.bin", chip_name);
- err = request_firmware(&adev->sdma.instance[i].fw, fw_name, adev->dev);
+ err = firmware_request_nowarn(&adev->sdma.instance[i].fw, fw_name, adev->dev);
if (err)
goto out;
err = amdgpu_ucode_validate(adev->sdma.instance[i].fw);
diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c b/drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c
index 521978c40537..75d2a9cc9268 100644
--- a/drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c
@@ -298,7 +298,7 @@ static int sdma_v3_0_init_microcode(struct amdgpu_device *adev)
snprintf(fw_name, sizeof(fw_name), "amdgpu/%s_sdma.bin", chip_name);
else
snprintf(fw_name, sizeof(fw_name), "amdgpu/%s_sdma1.bin", chip_name);
- err = request_firmware(&adev->sdma.instance[i].fw, fw_name, adev->dev);
+ err = firmware_request_nowarn(&adev->sdma.instance[i].fw, fw_name, adev->dev);
if (err)
goto out;
err = amdgpu_ucode_validate(adev->sdma.instance[i].fw);
diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c b/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
index 91cf95a8c39c..e1ebfb9e2650 100644
--- a/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
@@ -176,7 +176,7 @@ static int sdma_v4_0_init_microcode(struct amdgpu_device *adev)
snprintf(fw_name, sizeof(fw_name), "amdgpu/%s_sdma.bin", chip_name);
else
snprintf(fw_name, sizeof(fw_name), "amdgpu/%s_sdma1.bin", chip_name);
- err = request_firmware(&adev->sdma.instance[i].fw, fw_name, adev->dev);
+ err = firmware_request_nowarn(&adev->sdma.instance[i].fw, fw_name, adev->dev);
if (err)
goto out;
err = amdgpu_ucode_validate(adev->sdma.instance[i].fw);
diff --git a/drivers/gpu/drm/amd/amdgpu/si_dpm.c b/drivers/gpu/drm/amd/amdgpu/si_dpm.c
index ce675a7f179a..5538a5269417 100644
--- a/drivers/gpu/drm/amd/amdgpu/si_dpm.c
+++ b/drivers/gpu/drm/amd/amdgpu/si_dpm.c
@@ -7687,7 +7687,7 @@ static int si_dpm_init_microcode(struct amdgpu_device *adev)
}

snprintf(fw_name, sizeof(fw_name), "radeon/%s_smc.bin", chip_name);
- err = request_firmware(&adev->pm.fw, fw_name, adev->dev);
+ err = firmware_request_nowarn(&adev->pm.fw, fw_name, adev->dev);
if (err)
goto out;
err = amdgpu_ucode_validate(adev->pm.fw);
--
2.14.1

2018-04-17 15:36:43

[permalink] [raw]

Subject: [PATCH 1/9] firmware: some documentation fixes

Including:
- Fixup outdated kernel-doc paths
- Slightly too short title underline
- Some typos

Signed-off-by: Andres Rodriguez <[email protected]>
---
Documentation/driver-api/firmware/request_firmware.rst | 16 ++++++++--------
drivers/base/firmware_loader/fallback.c | 4 ++--
drivers/base/firmware_loader/fallback.h | 2 +-
3 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/Documentation/driver-api/firmware/request_firmware.rst b/Documentation/driver-api/firmware/request_firmware.rst
index 9305bf4db38e..7632f8807472 100644
--- a/Documentation/driver-api/firmware/request_firmware.rst
+++ b/Documentation/driver-api/firmware/request_firmware.rst
@@ -17,17 +17,17 @@ an error is returned.

firmware_request
----------------
-.. kernel-doc:: drivers/base/firmware_class.c
+.. kernel-doc:: drivers/base/firmware_loader/main.c
:functions: firmware_request

firmware_request_direct
-----------------------
-.. kernel-doc:: drivers/base/firmware_class.c
+.. kernel-doc:: drivers/base/firmware_loader/main.c
:functions: firmware_request_direct

firmware_request_into_buf
-------------------------
-.. kernel-doc:: drivers/base/firmware_class.c
+.. kernel-doc:: drivers/base/firmware_loader/main.c
:functions: firmware_request_into_buf

Asynchronous firmware requests
@@ -41,7 +41,7 @@ in atomic contexts.

firmware_request_nowait
-----------------------
-.. kernel-doc:: drivers/base/firmware_class.c
+.. kernel-doc:: drivers/base/firmware_loader/main.c
:functions: firmware_request_nowait

Special optimizations on reboot
@@ -50,12 +50,12 @@ Special optimizations on reboot
Some devices have an optimization in place to enable the firmware to be
retained during system reboot. When such optimizations are used the driver
author must ensure the firmware is still available on resume from suspend,
-this can be done with firmware_request_cache() insted of requesting for the
-firmare to be loaded.
+this can be done with firmware_request_cache() instead of requesting for the
+firmware to be loaded.

firmware_request_cache()
------------------------
-.. kernel-doc:: drivers/base/firmware_class.c
+------------------------
+.. kernel-doc:: drivers/base/firmware_loader/main.c
:functions: firmware_request_cache

request firmware API expected driver use
diff --git a/drivers/base/firmware_loader/fallback.c b/drivers/base/firmware_loader/fallback.c
index 71f529de5243..da97fc26119c 100644
--- a/drivers/base/firmware_loader/fallback.c
+++ b/drivers/base/firmware_loader/fallback.c
@@ -536,8 +536,8 @@ fw_create_instance(struct firmware *firmware, const char *fw_name,
}

/**
- * fw_load_sysfs_fallback - load a firmware via the syfs fallback mechanism
- * @fw_sysfs: firmware syfs information for the firmware to load
+ * fw_load_sysfs_fallback - load a firmware via the sysfs fallback mechanism
+ * @fw_sysfs: firmware sysfs information for the firmware to load
* @opt_flags: flags of options, FW_OPT_*
* @timeout: timeout to wait for the load
*
diff --git a/drivers/base/firmware_loader/fallback.h b/drivers/base/firmware_loader/fallback.h
index dfebc644ed35..f8255670a663 100644
--- a/drivers/base/firmware_loader/fallback.h
+++ b/drivers/base/firmware_loader/fallback.h
@@ -6,7 +6,7 @@
#include <linux/device.h>

/**
- * struct firmware_fallback_config - firmware fallback configuratioon settings
+ * struct firmware_fallback_config - firmware fallback configuration settings
*
* Helps describe and fine tune the fallback mechanism.
*
--
2.14.1

2018-04-17 15:36:46

[permalink] [raw]

Subject: [PATCH 4/9] firmware: use () to terminate kernel-doc function names

The kernel-doc spec dictates a function name ends in ().

Signed-off-by: Andres Rodriguez <[email protected]>
---
drivers/base/firmware_loader/fallback.c | 8 ++++----
drivers/base/firmware_loader/main.c | 22 +++++++++++-----------
2 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/drivers/base/firmware_loader/fallback.c b/drivers/base/firmware_loader/fallback.c
index ecc49a619298..e75928458489 100644
--- a/drivers/base/firmware_loader/fallback.c
+++ b/drivers/base/firmware_loader/fallback.c
@@ -124,7 +124,7 @@ static ssize_t timeout_show(struct class *class, struct class_attribute *attr,
}

/**
- * firmware_timeout_store - set number of seconds to wait for firmware
+ * firmware_timeout_store() - set number of seconds to wait for firmware
* @class: device class pointer
* @attr: device attribute pointer
* @buf: buffer to scan for timeout value
@@ -238,7 +238,7 @@ static int map_fw_priv_pages(struct fw_priv *fw_priv)
}

/**
- * firmware_loading_store - set value in the 'loading' control file
+ * firmware_loading_store() - set value in the 'loading' control file
* @dev: device pointer
* @attr: device attribute pointer
* @buf: buffer to scan for loading control value
@@ -430,7 +430,7 @@ static int fw_realloc_pages(struct fw_sysfs *fw_sysfs, int min_size)
}

/**
- * firmware_data_write - write method for firmware
+ * firmware_data_write() - write method for firmware
* @filp: open sysfs file
* @kobj: kobject for the device
* @bin_attr: bin_attr structure
@@ -536,7 +536,7 @@ fw_create_instance(struct firmware *firmware, const char *fw_name,
}

/**
- * fw_load_sysfs_fallback - load a firmware via the sysfs fallback mechanism
+ * fw_load_sysfs_fallback() - load a firmware via the sysfs fallback mechanism
* @fw_sysfs: firmware sysfs information for the firmware to load
* @opt_flags: flags of options, FW_OPT_*
* @timeout: timeout to wait for the load
diff --git a/drivers/base/firmware_loader/main.c b/drivers/base/firmware_loader/main.c
index 5baadad3012d..d028cd3257f7 100644
--- a/drivers/base/firmware_loader/main.c
+++ b/drivers/base/firmware_loader/main.c
@@ -597,7 +597,7 @@ _firmware_request(const struct firmware **firmware_p, const char *name,
}

/**
- * firmware_request: - send firmware request and wait for it
+ * firmware_request() - send firmware request and wait for it
* @firmware_p: pointer to firmware image
* @name: name of firmware file
* @device: device for which firmware is being loaded
@@ -632,7 +632,7 @@ firmware_request(const struct firmware **firmware_p, const char *name,
EXPORT_SYMBOL(firmware_request);

/**
- * firmware_request_direct: - load firmware directly without usermode helper
+ * firmware_request_direct() - load firmware directly without usermode helper
* @firmware_p: pointer to firmware image
* @name: name of firmware file
* @device: device for which firmware is being loaded
@@ -657,7 +657,7 @@ int firmware_request_direct(const struct firmware **firmware_p,
EXPORT_SYMBOL_GPL(firmware_request_direct);

/**
- * firmware_request_cache: - cache firmware for suspend so resume can use it
+ * firmware_request_cache() - cache firmware for suspend so resume can use it
* @name: name of firmware file
* @device: device for which firmware should be cached for
*
@@ -681,7 +681,7 @@ int firmware_request_cache(struct device *device, const char *name)
EXPORT_SYMBOL_GPL(firmware_request_cache);

/**
- * firmware_request_into_buf - load firmware into a previously allocated buffer
+ * firmware_request_into_buf() - load firmware into a previously allocated buffer
* @firmware_p: pointer to firmware image
* @name: name of firmware file
* @device: device for which firmware is being loaded and DMA region allocated
@@ -713,7 +713,7 @@ firmware_request_into_buf(const struct firmware **firmware_p, const char *name,
EXPORT_SYMBOL(firmware_request_into_buf);

/**
- * firmware_release: - release the resource associated with a firmware image
+ * firmware_release() - release the resource associated with a firmware image
* @fw: firmware resource to release
**/
void firmware_release(const struct firmware *fw)
@@ -755,7 +755,7 @@ static void firmware_request_work_func(struct work_struct *work)
}

/**
- * firmware_request_nowait - asynchronous version of firmware_request
+ * firmware_request_nowait() - asynchronous version of firmware_request
* @module: module requesting the firmware
* @uevent: sends uevent to copy the firmware image if this flag
* is non-zero else the firmware copy must be done manually.
@@ -824,7 +824,7 @@ EXPORT_SYMBOL(firmware_request_nowait);
static ASYNC_DOMAIN_EXCLUSIVE(fw_cache_domain);

/**
- * cache_firmware - cache one firmware image in kernel memory space
+ * cache_firmware() - cache one firmware image in kernel memory space
* @fw_name: the firmware image name
*
* Cache firmware in kernel memory so that drivers can use it when
@@ -866,7 +866,7 @@ static struct fw_priv *lookup_fw_priv(const char *fw_name)
}

/**
- * uncache_firmware - remove one cached firmware image
+ * uncache_firmware() - remove one cached firmware image
* @fw_name: the firmware image name
*
* Uncache one firmware image which has been cached successfully
@@ -1042,7 +1042,7 @@ static void __device_uncache_fw_images(void)
}

/**
- * device_cache_fw_images - cache devices' firmware
+ * device_cache_fw_images() - cache devices' firmware
*
* If one device called firmware_request or its nowait version
* successfully before, the firmware names are recored into the
@@ -1075,7 +1075,7 @@ static void device_cache_fw_images(void)
}

/**
- * device_uncache_fw_images - uncache devices' firmware
+ * device_uncache_fw_images() - uncache devices' firmware
*
* uncache all firmwares which have been cached successfully
* by device_uncache_fw_images earlier
@@ -1092,7 +1092,7 @@ static void device_uncache_fw_images_work(struct work_struct *work)
}

/**
- * device_uncache_fw_images_delay - uncache devices firmwares
+ * device_uncache_fw_images_delay() - uncache devices firmwares
* @delay: number of milliseconds to delay uncache device firmwares
*
* uncache all devices's firmwares which has been cached successfully
--
2.14.1

2018-04-17 15:37:20

[permalink] [raw]

Subject: [PATCH 3/9] firmware: add kernel-doc for enum fw_opt

Some basic definitions for the FW_OPT_* values

Signed-off-by: Andres Rodriguez <[email protected]>
---
drivers/base/firmware_loader/firmware.h | 11 +++++++++++
1 file changed, 11 insertions(+)

diff --git a/drivers/base/firmware_loader/firmware.h b/drivers/base/firmware_loader/firmware.h
index 957396293b92..8ef23c334135 100644
--- a/drivers/base/firmware_loader/firmware.h
+++ b/drivers/base/firmware_loader/firmware.h
@@ -10,6 +10,17 @@

#include <generated/utsrelease.h>

+/**
+ * enum fw_opt - options to control firmware loading behaviour
+ *
+ * @FW_OPT_UEVENT: Enable the uevent fallback path.
+ * @FW_OPT_NOWAIT: Executing inside an asynchronous firmware request.
+ * @FW_OPT_USERHELPER: Enable the usermode fallback path.
+ * @FW_OPT_NO_WARN: Quiet, avoid printing warning messages.
+ * @FW_OPT_NOCACHE: Firmware caching will be disabled for this request.
+ * @FW_OPT_NOFALLBACK: Disable the fallback path. Takes precedence over
+ * &FW_OPT_UEVENT and &FW_OPT_USERHELPER.
+ */
enum fw_opt {
FW_OPT_UEVENT = (1U << 0),
FW_OPT_NOWAIT = (1U << 1),
--
2.14.1

2018-04-17 15:37:35

[permalink] [raw]

Subject: [PATCH 2/9] firmware: wrap FW_OPT_* into an enum

This should let us associate enum kdoc to these values.

Signed-off-by: Andres Rodriguez <[email protected]>
---
drivers/base/firmware_loader/fallback.c | 12 ++++++------
drivers/base/firmware_loader/fallback.h | 6 ++++--
drivers/base/firmware_loader/firmware.h | 17 +++++++++--------
drivers/base/firmware_loader/main.c | 6 +++---
4 files changed, 22 insertions(+), 19 deletions(-)

diff --git a/drivers/base/firmware_loader/fallback.c b/drivers/base/firmware_loader/fallback.c
index da97fc26119c..ecc49a619298 100644
--- a/drivers/base/firmware_loader/fallback.c
+++ b/drivers/base/firmware_loader/fallback.c
@@ -511,7 +511,7 @@ static const struct attribute_group *fw_dev_attr_groups[] = {

static struct fw_sysfs *
fw_create_instance(struct firmware *firmware, const char *fw_name,
- struct device *device, unsigned int opt_flags)
+ struct device *device, enum fw_opt opt_flags)
{
struct fw_sysfs *fw_sysfs;
struct device *f_dev;
@@ -544,7 +544,7 @@ fw_create_instance(struct firmware *firmware, const char *fw_name,
* In charge of constructing a sysfs fallback interface for firmware loading.
**/
static int fw_load_sysfs_fallback(struct fw_sysfs *fw_sysfs,
- unsigned int opt_flags, long timeout)
+ enum fw_opt opt_flags, long timeout)
{
int retval = 0;
struct device *f_dev = &fw_sysfs->dev;
@@ -598,7 +598,7 @@ static int fw_load_sysfs_fallback(struct fw_sysfs *fw_sysfs,

static int fw_load_from_user_helper(struct firmware *firmware,
const char *name, struct device *device,
- unsigned int opt_flags)
+ enum fw_opt opt_flags)
{
struct fw_sysfs *fw_sysfs;
long timeout;
@@ -639,7 +639,7 @@ static int fw_load_from_user_helper(struct firmware *firmware,
return ret;
}

-static bool fw_force_sysfs_fallback(unsigned int opt_flags)
+static bool fw_force_sysfs_fallback(enum fw_opt opt_flags)
{
if (fw_fallback_config.force_sysfs_fallback)
return true;
@@ -648,7 +648,7 @@ static bool fw_force_sysfs_fallback(unsigned int opt_flags)
return true;
}

-static bool fw_run_sysfs_fallback(unsigned int opt_flags)
+static bool fw_run_sysfs_fallback(enum fw_opt opt_flags)
{
if (fw_fallback_config.ignore_sysfs_fallback) {
pr_info_once("Ignoring firmware sysfs fallback due to sysctl knob\n");
@@ -663,7 +663,7 @@ static bool fw_run_sysfs_fallback(unsigned int opt_flags)

int fw_sysfs_fallback(struct firmware *fw, const char *name,
struct device *device,
- unsigned int opt_flags,
+ enum fw_opt opt_flags,
int ret)
{
if (!fw_run_sysfs_fallback(opt_flags))
diff --git a/drivers/base/firmware_loader/fallback.h b/drivers/base/firmware_loader/fallback.h
index f8255670a663..a3b73a09db6c 100644
--- a/drivers/base/firmware_loader/fallback.h
+++ b/drivers/base/firmware_loader/fallback.h
@@ -5,6 +5,8 @@
#include <linux/firmware.h>
#include <linux/device.h>

+#include "firmware.h"
+
/**
* struct firmware_fallback_config - firmware fallback configuration settings
*
@@ -31,7 +33,7 @@ struct firmware_fallback_config {
#ifdef CONFIG_FW_LOADER_USER_HELPER
int fw_sysfs_fallback(struct firmware *fw, const char *name,
struct device *device,
- unsigned int opt_flags,
+ enum fw_opt opt_flags,
int ret);
void kill_pending_fw_fallback_reqs(bool only_kill_custom);

@@ -43,7 +45,7 @@ void unregister_sysfs_loader(void);
#else /* CONFIG_FW_LOADER_USER_HELPER */
static inline int fw_sysfs_fallback(struct firmware *fw, const char *name,
struct device *device,
- unsigned int opt_flags,
+ enum fw_opt opt_flags,
int ret)
{
/* Keep carrying over the same error */
diff --git a/drivers/base/firmware_loader/firmware.h b/drivers/base/firmware_loader/firmware.h
index d11d37db370b..957396293b92 100644
--- a/drivers/base/firmware_loader/firmware.h
+++ b/drivers/base/firmware_loader/firmware.h
@@ -10,13 +10,14 @@

#include <generated/utsrelease.h>

-/* firmware behavior options */
-#define FW_OPT_UEVENT (1U << 0)
-#define FW_OPT_NOWAIT (1U << 1)
-#define FW_OPT_USERHELPER (1U << 2)
-#define FW_OPT_NO_WARN (1U << 3)
-#define FW_OPT_NOCACHE (1U << 4)
-#define FW_OPT_NOFALLBACK (1U << 5)
+enum fw_opt {
+ FW_OPT_UEVENT = (1U << 0),
+ FW_OPT_NOWAIT = (1U << 1),
+ FW_OPT_USERHELPER = (1U << 2),
+ FW_OPT_NO_WARN = (1U << 3),
+ FW_OPT_NOCACHE = (1U << 4),
+ FW_OPT_NOFALLBACK = (1U << 5),
+};

enum fw_status {
FW_STATUS_UNKNOWN,
@@ -110,6 +111,6 @@ static inline void fw_state_done(struct fw_priv *fw_priv)
}

int assign_fw(struct firmware *fw, struct device *device,
- unsigned int opt_flags);
+ enum fw_opt opt_flags);

#endif /* __FIRMWARE_LOADER_H */
diff --git a/drivers/base/firmware_loader/main.c b/drivers/base/firmware_loader/main.c
index 5812148694b6..5baadad3012d 100644
--- a/drivers/base/firmware_loader/main.c
+++ b/drivers/base/firmware_loader/main.c
@@ -443,7 +443,7 @@ static int fw_add_devm_name(struct device *dev, const char *name)
#endif

int assign_fw(struct firmware *fw, struct device *device,
- unsigned int opt_flags)
+ enum fw_opt opt_flags)
{
struct fw_priv *fw_priv = fw->priv;
int ret;
@@ -558,7 +558,7 @@ static void fw_abort_batch_reqs(struct firmware *fw)
static int
_firmware_request(const struct firmware **firmware_p, const char *name,
struct device *device, void *buf, size_t size,
- unsigned int opt_flags)
+ enum fw_opt opt_flags)
{
struct firmware *fw = NULL;
int ret;
@@ -734,7 +734,7 @@ struct firmware_work {
struct device *device;
void *context;
void (*cont)(const struct firmware *fw, void *context);
- unsigned int opt_flags;
+ enum fw_opt opt_flags;
};

static void firmware_request_work_func(struct work_struct *work)
--
2.14.1

2018-04-17 15:38:31

[permalink] [raw]

Subject: [PATCH 5/9] firmware: add functions to load firmware without warnings v4

Currently the firmware loader only exposes one silent path for querying
optional firmware, and that is firmware_request_direct(). This function
also disables the fallback path, which might not always be the
desired behaviour. [0]

This patch introduces variations of firmware_request() and
firmware_request_nowait() that enable the caller to disable the
undesired warning messages. This is equivalent to adding FW_OPT_NO_WARN,
to the old behaviour.

v2: add header prototype, use updated opt_flags
v3: split debug message to separate patch
added _nowait variant
added documentation
v4: fix kernel-doc style

[0]: https://git.kernel.org/linus/c0cc00f250e1

Signed-off-by: Andres Rodriguez <[email protected]>
---
.../driver-api/firmware/request_firmware.rst | 13 ++++--
drivers/base/firmware_loader/main.c | 52 ++++++++++++++++++++--
include/linux/firmware.h | 6 +++
3 files changed, 63 insertions(+), 8 deletions(-)

diff --git a/Documentation/driver-api/firmware/request_firmware.rst b/Documentation/driver-api/firmware/request_firmware.rst
index 7632f8807472..913e7d2e0cb7 100644
--- a/Documentation/driver-api/firmware/request_firmware.rst
+++ b/Documentation/driver-api/firmware/request_firmware.rst
@@ -20,6 +20,11 @@ firmware_request
.. kernel-doc:: drivers/base/firmware_loader/main.c
:functions: firmware_request

+firmware_request_nowarn
+-----------------------
+.. kernel-doc:: drivers/base/firmware_loader/main.c
+ :functions: firmware_request_nowarn
+
firmware_request_direct
-----------------------
.. kernel-doc:: drivers/base/firmware_loader/main.c
@@ -36,13 +41,13 @@ Asynchronous firmware requests
Asynchronous firmware requests allow driver code to not have to wait
until the firmware or an error is returned. Function callbacks are
provided so that when the firmware or an error is found the driver is
-informed through the callback. firmware_request_nowait() cannot be called
+informed through the callback. firmware_request_nowait2() cannot be called
in atomic contexts.

-firmware_request_nowait
------------------------
+firmware_request_nowait2
+------------------------
.. kernel-doc:: drivers/base/firmware_loader/main.c
- :functions: firmware_request_nowait
+ :functions: firmware_request_nowait2

Special optimizations on reboot
===============================
diff --git a/drivers/base/firmware_loader/main.c b/drivers/base/firmware_loader/main.c
index d028cd3257f7..e449ac990dc9 100644
--- a/drivers/base/firmware_loader/main.c
+++ b/drivers/base/firmware_loader/main.c
@@ -631,6 +631,30 @@ firmware_request(const struct firmware **firmware_p, const char *name,
}
EXPORT_SYMBOL(firmware_request);

+/**
+ * firmware_request_nowarn() - request for an optional fw module
+ * @firmware: pointer to firmware image
+ * @name: name of firmware file
+ * @device: device for which firmware is being loaded
+ *
+ * This function is similar in behaviour to firmware_request(), except
+ * it doesn't produce warning messages when the file is not found.
+ **/
+int
+firmware_request_nowarn(const struct firmware **firmware, const char *name,
+ struct device *device)
+{
+ int ret;
+
+ /* Need to pin this module until return */
+ __module_get(THIS_MODULE);
+ ret = _firmware_request(firmware, name, device, NULL, 0,
+ FW_OPT_UEVENT | FW_OPT_NO_WARN);
+ module_put(THIS_MODULE);
+ return ret;
+}
+EXPORT_SYMBOL_GPL(firmware_request_nowarn);
+
/**
* firmware_request_direct() - load firmware directly without usermode helper
* @firmware_p: pointer to firmware image
@@ -755,10 +779,11 @@ static void firmware_request_work_func(struct work_struct *work)
}

/**
- * firmware_request_nowait() - asynchronous version of firmware_request
+ * firmware_request_nowait2() - asynchronous version of firmware_request
* @module: module requesting the firmware
* @uevent: sends uevent to copy the firmware image if this flag
* is non-zero else the firmware copy must be done manually.
+ * @warn: enable warnings
* @name: name of firmware file
* @device: device for which firmware is being loaded
* @gfp: allocation flags
@@ -778,8 +803,8 @@ static void firmware_request_work_func(struct work_struct *work)
* - can't sleep at all if @gfp is GFP_ATOMIC.
**/
int
-firmware_request_nowait(
- struct module *module, bool uevent,
+firmware_request_nowait2(
+ struct module *module, bool uevent, bool warn,
const char *name, struct device *device, gfp_t gfp, void *context,
void (*cont)(const struct firmware *fw, void *context))
{
@@ -799,7 +824,8 @@ firmware_request_nowait(
fw_work->context = context;
fw_work->cont = cont;
fw_work->opt_flags = FW_OPT_NOWAIT |
- (uevent ? FW_OPT_UEVENT : FW_OPT_USERHELPER);
+ (uevent ? FW_OPT_UEVENT : FW_OPT_USERHELPER) |
+ (warn ? 0 : FW_OPT_NO_WARN);

if (!uevent && fw_cache_is_setup(device, name)) {
kfree_const(fw_work->name);
@@ -818,6 +844,24 @@ firmware_request_nowait(
schedule_work(&fw_work->work);
return 0;
}
+EXPORT_SYMBOL_GPL(firmware_request_nowait2);
+
+/**
+ * firmware_request_nowait() - compatibility version of firmware_request_nowait2
+ *
+ * This is equivalent to calling firmware_request_nowait2 with warnings enabled.
+ *
+ * Refer to firmware_request_nowait2 for further details.
+ **/
+int
+firmware_request_nowait(
+ struct module *module, bool uevent,
+ const char *name, struct device *device, gfp_t gfp, void *context,
+ void (*cont)(const struct firmware *fw, void *context))
+{
+ return firmware_request_nowait2(module, uevent, true, name, device,
+ gfp, context, cont);
+}
EXPORT_SYMBOL(firmware_request_nowait);

#ifdef CONFIG_PM_SLEEP
diff --git a/include/linux/firmware.h b/include/linux/firmware.h
index db8351a42405..2623d3f853ba 100644
--- a/include/linux/firmware.h
+++ b/include/linux/firmware.h
@@ -42,6 +42,12 @@ struct builtin_fw {
#if defined(CONFIG_FW_LOADER) || (defined(CONFIG_FW_LOADER_MODULE) && defined(MODULE))
int firmware_request(const struct firmware **fw, const char *name,
struct device *device);
+int firmware_request_nowarn(const struct firmware **fw, const char *name,
+ struct device *device);
+int firmware_request_nowait2(
+ struct module *module, bool uevent, bool warn,
+ const char *name, struct device *device, gfp_t gfp, void *context,
+ void (*cont)(const struct firmware *fw, void *context));
int firmware_request_nowait(
struct module *module, bool uevent,
const char *name, struct device *device, gfp_t gfp, void *context,
--
2.14.1

2018-04-17 20:58:23

by Randy Dunlap

[permalink] [raw]

Subject: Re: [PATCH 4/9] firmware: use () to terminate kernel-doc function names

On 04/17/18 08:33, Andres Rodriguez wrote:
> The kernel-doc spec dictates a function name ends in ().
>
> Signed-off-by: Andres Rodriguez <[email protected]>

Acked-by: Randy Dunlap <[email protected]>

Thanks.

> ---
> drivers/base/firmware_loader/fallback.c | 8 ++++----
> drivers/base/firmware_loader/main.c | 22 +++++++++++-----------
> 2 files changed, 15 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/base/firmware_loader/fallback.c b/drivers/base/firmware_loader/fallback.c
> index ecc49a619298..e75928458489 100644
> --- a/drivers/base/firmware_loader/fallback.c
> +++ b/drivers/base/firmware_loader/fallback.c
> @@ -124,7 +124,7 @@ static ssize_t timeout_show(struct class *class, struct class_attribute *attr,
> }
>
> /**
> - * firmware_timeout_store - set number of seconds to wait for firmware
> + * firmware_timeout_store() - set number of seconds to wait for firmware
> * @class: device class pointer
> * @attr: device attribute pointer
> * @buf: buffer to scan for timeout value
> @@ -238,7 +238,7 @@ static int map_fw_priv_pages(struct fw_priv *fw_priv)
> }
>
> /**
> - * firmware_loading_store - set value in the 'loading' control file
> + * firmware_loading_store() - set value in the 'loading' control file
> * @dev: device pointer
> * @attr: device attribute pointer
> * @buf: buffer to scan for loading control value
> @@ -430,7 +430,7 @@ static int fw_realloc_pages(struct fw_sysfs *fw_sysfs, int min_size)
> }
>
> /**
> - * firmware_data_write - write method for firmware
> + * firmware_data_write() - write method for firmware
> * @filp: open sysfs file
> * @kobj: kobject for the device
> * @bin_attr: bin_attr structure
> @@ -536,7 +536,7 @@ fw_create_instance(struct firmware *firmware, const char *fw_name,
> }
>
> /**
> - * fw_load_sysfs_fallback - load a firmware via the sysfs fallback mechanism
> + * fw_load_sysfs_fallback() - load a firmware via the sysfs fallback mechanism
> * @fw_sysfs: firmware sysfs information for the firmware to load
> * @opt_flags: flags of options, FW_OPT_*
> * @timeout: timeout to wait for the load
> diff --git a/drivers/base/firmware_loader/main.c b/drivers/base/firmware_loader/main.c
> index 5baadad3012d..d028cd3257f7 100644
> --- a/drivers/base/firmware_loader/main.c
> +++ b/drivers/base/firmware_loader/main.c
> @@ -597,7 +597,7 @@ _firmware_request(const struct firmware **firmware_p, const char *name,
> }
>
> /**
> - * firmware_request: - send firmware request and wait for it
> + * firmware_request() - send firmware request and wait for it
> * @firmware_p: pointer to firmware image
> * @name: name of firmware file
> * @device: device for which firmware is being loaded
> @@ -632,7 +632,7 @@ firmware_request(const struct firmware **firmware_p, const char *name,
> EXPORT_SYMBOL(firmware_request);
>
> /**
> - * firmware_request_direct: - load firmware directly without usermode helper
> + * firmware_request_direct() - load firmware directly without usermode helper
> * @firmware_p: pointer to firmware image
> * @name: name of firmware file
> * @device: device for which firmware is being loaded
> @@ -657,7 +657,7 @@ int firmware_request_direct(const struct firmware **firmware_p,
> EXPORT_SYMBOL_GPL(firmware_request_direct);
>
> /**
> - * firmware_request_cache: - cache firmware for suspend so resume can use it
> + * firmware_request_cache() - cache firmware for suspend so resume can use it
> * @name: name of firmware file
> * @device: device for which firmware should be cached for
> *
> @@ -681,7 +681,7 @@ int firmware_request_cache(struct device *device, const char *name)
> EXPORT_SYMBOL_GPL(firmware_request_cache);
>
> /**
> - * firmware_request_into_buf - load firmware into a previously allocated buffer
> + * firmware_request_into_buf() - load firmware into a previously allocated buffer
> * @firmware_p: pointer to firmware image
> * @name: name of firmware file
> * @device: device for which firmware is being loaded and DMA region allocated
> @@ -713,7 +713,7 @@ firmware_request_into_buf(const struct firmware **firmware_p, const char *name,
> EXPORT_SYMBOL(firmware_request_into_buf);
>
> /**
> - * firmware_release: - release the resource associated with a firmware image
> + * firmware_release() - release the resource associated with a firmware image
> * @fw: firmware resource to release
> **/
> void firmware_release(const struct firmware *fw)
> @@ -755,7 +755,7 @@ static void firmware_request_work_func(struct work_struct *work)
> }
>
> /**
> - * firmware_request_nowait - asynchronous version of firmware_request
> + * firmware_request_nowait() - asynchronous version of firmware_request
> * @module: module requesting the firmware
> * @uevent: sends uevent to copy the firmware image if this flag
> * is non-zero else the firmware copy must be done manually.
> @@ -824,7 +824,7 @@ EXPORT_SYMBOL(firmware_request_nowait);
> static ASYNC_DOMAIN_EXCLUSIVE(fw_cache_domain);
>
> /**
> - * cache_firmware - cache one firmware image in kernel memory space
> + * cache_firmware() - cache one firmware image in kernel memory space
> * @fw_name: the firmware image name
> *
> * Cache firmware in kernel memory so that drivers can use it when
> @@ -866,7 +866,7 @@ static struct fw_priv *lookup_fw_priv(const char *fw_name)
> }
>
> /**
> - * uncache_firmware - remove one cached firmware image
> + * uncache_firmware() - remove one cached firmware image
> * @fw_name: the firmware image name
> *
> * Uncache one firmware image which has been cached successfully
> @@ -1042,7 +1042,7 @@ static void __device_uncache_fw_images(void)
> }
>
> /**
> - * device_cache_fw_images - cache devices' firmware
> + * device_cache_fw_images() - cache devices' firmware
> *
> * If one device called firmware_request or its nowait version
> * successfully before, the firmware names are recored into the
> @@ -1075,7 +1075,7 @@ static void device_cache_fw_images(void)
> }
>
> /**
> - * device_uncache_fw_images - uncache devices' firmware
> + * device_uncache_fw_images() - uncache devices' firmware
> *
> * uncache all firmwares which have been cached successfully
> * by device_uncache_fw_images earlier
> @@ -1092,7 +1092,7 @@ static void device_uncache_fw_images_work(struct work_struct *work)
> }
>
> /**
> - * device_uncache_fw_images_delay - uncache devices firmwares
> + * device_uncache_fw_images_delay() - uncache devices firmwares
> * @delay: number of milliseconds to delay uncache device firmwares
> *
> * uncache all devices's firmwares which has been cached successfully
>

--
~Randy

2018-04-17 21:02:42

by Randy Dunlap

[permalink] [raw]

Subject: Re: [PATCH 1/9] firmware: some documentation fixes

On 04/17/18 08:32, Andres Rodriguez wrote:
> Including:
> - Fixup outdated kernel-doc paths
> - Slightly too short title underline
> - Some typos
>
> Signed-off-by: Andres Rodriguez <[email protected]>

BTW, Hans de Goede <[email protected]> has already submitted a patch to
handle the file rename(s) for Documentation.

> ---
> Documentation/driver-api/firmware/request_firmware.rst | 16 ++++++++--------
> drivers/base/firmware_loader/fallback.c | 4 ++--
> drivers/base/firmware_loader/fallback.h | 2 +-
> 3 files changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/Documentation/driver-api/firmware/request_firmware.rst b/Documentation/driver-api/firmware/request_firmware.rst
> index 9305bf4db38e..7632f8807472 100644
> --- a/Documentation/driver-api/firmware/request_firmware.rst
> +++ b/Documentation/driver-api/firmware/request_firmware.rst
> @@ -17,17 +17,17 @@ an error is returned.
>
> firmware_request
> ----------------
> -.. kernel-doc:: drivers/base/firmware_class.c
> +.. kernel-doc:: drivers/base/firmware_loader/main.c
> :functions: firmware_request
>
> firmware_request_direct
> -----------------------
> -.. kernel-doc:: drivers/base/firmware_class.c
> +.. kernel-doc:: drivers/base/firmware_loader/main.c
> :functions: firmware_request_direct
>
> firmware_request_into_buf
> -------------------------
> -.. kernel-doc:: drivers/base/firmware_class.c
> +.. kernel-doc:: drivers/base/firmware_loader/main.c
> :functions: firmware_request_into_buf
>
> Asynchronous firmware requests
> @@ -41,7 +41,7 @@ in atomic contexts.
>
> firmware_request_nowait
> -----------------------
> -.. kernel-doc:: drivers/base/firmware_class.c
> +.. kernel-doc:: drivers/base/firmware_loader/main.c
> :functions: firmware_request_nowait
>
> Special optimizations on reboot
> @@ -50,12 +50,12 @@ Special optimizations on reboot
> Some devices have an optimization in place to enable the firmware to be
> retained during system reboot. When such optimizations are used the driver
> author must ensure the firmware is still available on resume from suspend,
> -this can be done with firmware_request_cache() insted of requesting for the
> -firmare to be loaded.
> +this can be done with firmware_request_cache() instead of requesting for the
> +firmware to be loaded.
>
> firmware_request_cache()
> ------------------------
> -.. kernel-doc:: drivers/base/firmware_class.c
> +------------------------
> +.. kernel-doc:: drivers/base/firmware_loader/main.c
> :functions: firmware_request_cache
>
> request firmware API expected driver use
> diff --git a/drivers/base/firmware_loader/fallback.c b/drivers/base/firmware_loader/fallback.c
> index 71f529de5243..da97fc26119c 100644
> --- a/drivers/base/firmware_loader/fallback.c
> +++ b/drivers/base/firmware_loader/fallback.c
> @@ -536,8 +536,8 @@ fw_create_instance(struct firmware *firmware, const char *fw_name,
> }
>
> /**
> - * fw_load_sysfs_fallback - load a firmware via the syfs fallback mechanism
> - * @fw_sysfs: firmware syfs information for the firmware to load
> + * fw_load_sysfs_fallback - load a firmware via the sysfs fallback mechanism
> + * @fw_sysfs: firmware sysfs information for the firmware to load
> * @opt_flags: flags of options, FW_OPT_*
> * @timeout: timeout to wait for the load
> *
> diff --git a/drivers/base/firmware_loader/fallback.h b/drivers/base/firmware_loader/fallback.h
> index dfebc644ed35..f8255670a663 100644
> --- a/drivers/base/firmware_loader/fallback.h
> +++ b/drivers/base/firmware_loader/fallback.h
> @@ -6,7 +6,7 @@
> #include <linux/device.h>
>
> /**
> - * struct firmware_fallback_config - firmware fallback configuratioon settings
> + * struct firmware_fallback_config - firmware fallback configuration settings
> *
> * Helps describe and fine tune the fallback mechanism.
> *
>

--
~Randy

2018-04-20 10:20:41

[permalink] [raw]

Subject: Re: [PATCH 8/9] ath10k: use request_firmware_nowarn to load firmware

(adding linux-wireless and ath10k lists)

Andres Rodriguez <[email protected]> writes:

> This reduces the unnecessary spew when trying to load optional firmware:
> "Direct firmware load for ... failed with error -2"
>
> Signed-off-by: Andres Rodriguez <[email protected]>
> ---
> drivers/net/wireless/ath/ath10k/core.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)

Acked-by: Kalle Valo <[email protected]>

But I think it would be good to change also request_firmware_direct() in
ath10k/testmode.c to request_firmware_nowarn().

--
Kalle Valo

2018-04-20 10:28:36

[permalink] [raw]

Subject: Re: [PATCH 9/9] brcmfmac: use request_firmware_nowait2 to load firmware without warnings

Andres Rodriguez <[email protected]> writes:

> This reduces the unnecessary spew when trying to load optional firmware:
> "Direct firmware load for ... failed with error -2"
>
> Signed-off-by: Andres Rodriguez <[email protected]>
> ---
> drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)

With wireless patches always CC linux-wireless list, please. Adding it
now.

> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
> index 091b52979e03..26db3ebd52dc 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
> @@ -503,8 +503,9 @@ static void brcmf_fw_request_code_done(const struct firmware *fw, void *ctx)
> goto done;
>
> fwctx->code = fw;
> - ret = request_firmware_nowait(THIS_MODULE, true, fwctx->nvram_name,
> - fwctx->dev, GFP_KERNEL, fwctx,
> + ret = request_firmware_nowait(THIS_MODULE, true, false,

A perfect example why enums should be in function calls instead of
booleans, that "true, false" tells nothing to me and it would be time
consuming to check from headers files what it means. If you had proper
enums, for example "FIRMWARE_MODE_FOO, FIRMWARE_STATE_BAR", it would be
immediately obvious for the reader what the parameters are. Of course
the first boolean was already there before, but maybe change the new
boolean to an enum?

> + fwctx->nvram_name, fwctx->dev,
> + GFP_KERNEL, fwctx,
> brcmf_fw_request_nvram_done);
>
> /* pass NULL to nvram callback for bcm47xx fallback */
> @@ -547,7 +548,7 @@ int brcmf_fw_get_firmwares_pcie(struct device *dev, u16 flags,
> fwctx->domain_nr = domain_nr;
> fwctx->bus_nr = bus_nr;
>
> - return request_firmware_nowait(THIS_MODULE, true, code, dev,
> + return request_firmware_nowait2(THIS_MODULE, true, false, code, dev,
> GFP_KERNEL, fwctx,
> brcmf_fw_request_code_done);
> }

Also the number two in the function name is not really telling anything.
I think that something like request_firmware_nowait_nowarn() would be
better, even if it's so ugly.

--
Kalle Valo

2018-04-20 10:30:17

[permalink] [raw]

Subject: Re: [PATCH 5/9] firmware: add functions to load firmware without warnings v4

Andres Rodriguez <[email protected]> writes:

> Currently the firmware loader only exposes one silent path for querying
> optional firmware, and that is firmware_request_direct(). This function
> also disables the fallback path, which might not always be the
> desired behaviour. [0]
>
> This patch introduces variations of firmware_request() and
> firmware_request_nowait() that enable the caller to disable the
> undesired warning messages. This is equivalent to adding FW_OPT_NO_WARN,
> to the old behaviour.
>
> v2: add header prototype, use updated opt_flags
> v3: split debug message to separate patch
> added _nowait variant
> added documentation
> v4: fix kernel-doc style
>
> [0]: https://git.kernel.org/linus/c0cc00f250e1
>
> Signed-off-by: Andres Rodriguez <[email protected]>
> ---
> .../driver-api/firmware/request_firmware.rst | 13 ++++--
> drivers/base/firmware_loader/main.c | 52 ++++++++++++++++++++--
> include/linux/firmware.h | 6 +++
> 3 files changed, 63 insertions(+), 8 deletions(-)

The changelog should be after the "---" line, this way git-am will
automatically discard it.

--
Kalle Valo

2018-04-20 19:35:04

[permalink] [raw]

Subject: Re: [PATCH 9/9] brcmfmac: use request_firmware_nowait2 to load firmware without warnings

On 2018-04-20 06:26 AM, Kalle Valo wrote:
> Andres Rodriguez <[email protected]> writes:
>
>> This reduces the unnecessary spew when trying to load optional firmware:
>> "Direct firmware load for ... failed with error -2"
>>
>> Signed-off-by: Andres Rodriguez <[email protected]>
>> ---
>> drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c | 7 ++++---
>> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> With wireless patches always CC linux-wireless list, please. Adding it
> now.
>
>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
>> index 091b52979e03..26db3ebd52dc 100644
>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
>> @@ -503,8 +503,9 @@ static void brcmf_fw_request_code_done(const struct firmware *fw, void *ctx)
>> goto done;
>>
>> fwctx->code = fw;
>> - ret = request_firmware_nowait(THIS_MODULE, true, fwctx->nvram_name,
>> - fwctx->dev, GFP_KERNEL, fwctx,
>> + ret = request_firmware_nowait(THIS_MODULE, true, false,
>
> A perfect example why enums should be in function calls instead of
> booleans, that "true, false" tells nothing to me and it would be time
> consuming to check from headers files what it means. If you had proper
> enums, for example "FIRMWARE_MODE_FOO, FIRMWARE_STATE_BAR", it would be
> immediately obvious for the reader what the parameters are. Of course
> the first boolean was already there before, but maybe change the new
> boolean to an enum >

Anyone else got any feedback before I re-spin the _nowait() API. I'm on
board for the boolean->enum change.

>> + fwctx->nvram_name, fwctx->dev,
>> + GFP_KERNEL, fwctx,
>> brcmf_fw_request_nvram_done);
>>
>> /* pass NULL to nvram callback for bcm47xx fallback */
>> @@ -547,7 +548,7 @@ int brcmf_fw_get_firmwares_pcie(struct device *dev, u16 flags,
>> fwctx->domain_nr = domain_nr;
>> fwctx->bus_nr = bus_nr;
>>
>> - return request_firmware_nowait(THIS_MODULE, true, code, dev,
>> + return request_firmware_nowait2(THIS_MODULE, true, false, code, dev,
>> GFP_KERNEL, fwctx,
>> brcmf_fw_request_code_done);
>> }
>
> Also the number two in the function name is not really telling anything.
> I think that something like request_firmware_nowait_nowarn() would be
> better, even if it's so ugly.
>

The 2 is meant to signify that this is an new version of the api with
different parameters. I don't think we need to codify into the name what
the new parameters mean (mostly because when a 2 version of an api is
implemented, usage of the original version tends to be discouraged).

If we go for something like _nowait_nowarn(), then we would need to drop
the warn parameter altogether.

Another alternative, drop both bool warn and bool uevent and expose take
in enum fw_opt directly.

Any thought on exposing the enum directly Luis for _nowait(). I know you
mentioned this was for some reason decided against for the rest of the API.

Regards,
Andres

2018-04-21 07:24:04

[permalink] [raw]

Subject: Re: [PATCH 9/9] brcmfmac: use request_firmware_nowait2 to load firmware without warnings

Andres Rodriguez <[email protected]> writes:

>>> + fwctx->nvram_name, fwctx->dev,
>>> + GFP_KERNEL, fwctx,
>>> brcmf_fw_request_nvram_done);
>>> /* pass NULL to nvram callback for bcm47xx fallback */
>>> @@ -547,7 +548,7 @@ int brcmf_fw_get_firmwares_pcie(struct device *dev, u16 flags,
>>> fwctx->domain_nr = domain_nr;
>>> fwctx->bus_nr = bus_nr;
>>> - return request_firmware_nowait(THIS_MODULE, true, code, dev,
>>> + return request_firmware_nowait2(THIS_MODULE, true, false, code, dev,
>>> GFP_KERNEL, fwctx,
>>> brcmf_fw_request_code_done);
>>> }
>>
>> Also the number two in the function name is not really telling anything.
>> I think that something like request_firmware_nowait_nowarn() would be
>> better, even if it's so ugly.
>>
>
> The 2 is meant to signify that this is an new version of the api with
> different parameters.

Ah, I missed that. I didn't have time to review your patches in detail,
just looked at the wireless patches.

> I don't think we need to codify into the name what the new parameters
> mean (mostly because when a 2 version of an api is implemented, usage
> of the original version tends to be discouraged).

Yeah, makes sense now.

--
Kalle Valo

2018-04-21 08:08:31

by Arend Van Spriel

[permalink] [raw]

Subject: Re: [PATCH 9/9] brcmfmac: use request_firmware_nowait2 to load firmware without warnings

On 4/20/2018 12:26 PM, Kalle Valo wrote:
> Andres Rodriguez <[email protected]> writes:
>
>> This reduces the unnecessary spew when trying to load optional firmware:
>> "Direct firmware load for ... failed with error -2"

So what happened with the request_firmware_nowarn() api (discussed in
another thread). Did it get lost with your kidney stones ;-) ? It seems
we start having the same discussion about the asynchronous variant as
well here which is a bit counter productive.

Let's get back to the issue of the message above. So when is the message
unnecessary. To me there are actually to cases in which the message can
confuse people searching the log for hints on a issue they have with a
device. 1) when the driver requests a sequence of files and only needs
one, and 2) when the driver request can be handled by fallback. Why not
only issue the error message when the device driver uses
request_firmware_direct() or when there is no fallback.

Also this patch does not seem to be made against latest code as I did a
major rework that went in v4.17-rc1.

>> Signed-off-by: Andres Rodriguez <[email protected]>
>> ---
>> drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c | 7 ++++---
>> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> With wireless patches always CC linux-wireless list, please. Adding it
> now.
>
>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
>> index 091b52979e03..26db3ebd52dc 100644
>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
>> @@ -503,8 +503,9 @@ static void brcmf_fw_request_code_done(const struct firmware *fw, void *ctx)
>> goto done;
>>
>> fwctx->code = fw;
>> - ret = request_firmware_nowait(THIS_MODULE, true, fwctx->nvram_name,
>> - fwctx->dev, GFP_KERNEL, fwctx,
>> + ret = request_firmware_nowait(THIS_MODULE, true, false,
>
> A perfect example why enums should be in function calls instead of
> booleans, that "true, false" tells nothing to me and it would be time
> consuming to check from headers files what it means. If you had proper
> enums, for example "FIRMWARE_MODE_FOO, FIRMWARE_STATE_BAR", it would be
> immediately obvious for the reader what the parameters are. Of course
> the first boolean was already there before, but maybe change the new
> boolean to an enum?

I can not fully agree here. While being a bit more descriptive even with
enums wrong enum values can be used due to copy-paste errors for
instance. Also when reviewing code, sometime looking up function
prototypes and type definitions are part of the fun. Tools like ctags or
elixir website make it pretty easy.

Now regarding this part of the patch the driver is requesting nvram
file, which is not always optional. For SDIO devices it is required and
for PCIe it is optional so firmware.c module is instructed about this
with a flag. So here that flag should be used to pass the proper
boolean/call the appropriate function. Actually in the latest code the
nvram is request synchronously.

>> + fwctx->nvram_name, fwctx->dev,
>> + GFP_KERNEL, fwctx,
>> brcmf_fw_request_nvram_done);
>>
>> /* pass NULL to nvram callback for bcm47xx fallback */
>> @@ -547,7 +548,7 @@ int brcmf_fw_get_firmwares_pcie(struct device *dev, u16 flags,
>> fwctx->domain_nr = domain_nr;
>> fwctx->bus_nr = bus_nr;
>>
>> - return request_firmware_nowait(THIS_MODULE, true, code, dev,
>> + return request_firmware_nowait2(THIS_MODULE, true, false, code, dev,
>> GFP_KERNEL, fwctx,
>> brcmf_fw_request_code_done);
>> }
>
> Also the number two in the function name is not really telling anything.
> I think that something like request_firmware_nowait_nowarn() would be
> better, even if it's so ugly.

This is requesting the actual firmware that is run by the cpu on the
chip so it is not optional.

Again, the firmware.c module has been reworked quite a bit in v4.17-rc1
so this patch is outdated.

Regards,
Arend

2018-04-21 14:01:39

[permalink] [raw]

Subject: Re: [PATCH 2/9] firmware: wrap FW_OPT_* into an enum

On Tue, Apr 17, 2018 at 11:33:00AM -0400, Andres Rodriguez wrote:
> This should let us associate enum kdoc to these values.
>
> Signed-off-by: Andres Rodriguez <[email protected]>
> ---
> diff --git a/drivers/base/firmware_loader/firmware.h b/drivers/base/firmware_loader/firmware.h
> index d11d37db370b..957396293b92 100644
> --- a/drivers/base/firmware_loader/firmware.h
> +++ b/drivers/base/firmware_loader/firmware.h
> @@ -10,13 +10,14 @@
>
> #include <generated/utsrelease.h>
>
> -/* firmware behavior options */
> -#define FW_OPT_UEVENT (1U << 0)
> -#define FW_OPT_NOWAIT (1U << 1)
> -#define FW_OPT_USERHELPER (1U << 2)
> -#define FW_OPT_NO_WARN (1U << 3)
> -#define FW_OPT_NOCACHE (1U << 4)
> -#define FW_OPT_NOFALLBACK (1U << 5)
> +enum fw_opt {
> + FW_OPT_UEVENT = (1U << 0),
> + FW_OPT_NOWAIT = (1U << 1),
> + FW_OPT_USERHELPER = (1U << 2),
> + FW_OPT_NO_WARN = (1U << 3),
> + FW_OPT_NOCACHE = (1U << 4),
> + FW_OPT_NOFALLBACK = (1U << 5),
> +};

Please use BIT(i) and include linux/bitops.h instead.

Luis

2018-04-21 14:32:58

[permalink] [raw]

Subject: Re: [PATCH 3/9] firmware: add kernel-doc for enum fw_opt

On Tue, Apr 17, 2018 at 11:33:01AM -0400, Andres Rodriguez wrote:
> Some basic definitions for the FW_OPT_* values
>
> Signed-off-by: Andres Rodriguez <[email protected]>
> ---
> drivers/base/firmware_loader/firmware.h | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
> diff --git a/drivers/base/firmware_loader/firmware.h b/drivers/base/firmware_loader/firmware.h
> index 957396293b92..8ef23c334135 100644
> --- a/drivers/base/firmware_loader/firmware.h
> +++ b/drivers/base/firmware_loader/firmware.h
> @@ -10,6 +10,17 @@
>
> #include <generated/utsrelease.h>
>
> +/**
> + * enum fw_opt - options to control firmware loading behaviour
> + *
> + * @FW_OPT_UEVENT: Enable the uevent fallback path.

No, that is not what this does. This sends a kobject uevent to userspace when the
fallback mechanism is used. So please use:

"Enables the fallback mechanism to send a kobject uevent when the firmware
is not found. Userspace is in charge to load the firmware using the sysfs
loading facility."

> + * @FW_OPT_NOWAIT: Executing inside an asynchronous firmware request.

Please use "Used to describe the firmware request is asynchronous.".

> + * @FW_OPT_USERHELPER: Enable the usermode fallback path.

The notion of "userhelper" only causes confusion since the kernel has its
own usermode helper (kernel/umh.c), and the only use case that the firmware loader
has for it is for when CONFIG_UEVENT_HELPER_PATH is set to call out to
userspace helper for kobject uevents. In practice *no one* is setting or using
this these days.

So I have been going on a small nomenclature crusade to clean this mess up bon
on kernel/umh.c and the firmware loader. This is why the entire fallback mechanism
is now stuffed into a file called fallback.c. I don't want to confuse people
further so please do not refer to "usermode" anymore for the fallback mechanism,
it is completely misleading. We should eventually just rename this flag to
fallback or something.

Therefore please be consistent with the documentation and use:

"Enable the fallback mechanism, in case the direct filesystem lookup
fails at finding the firmware. For details refer to fw_sysfs_fallback()."

Since fw_sysfs_fallback() is used in documentation and we don't want to clash
with other documentation names, then rename fw_sysfs_fallback() to use the
firmware_ prefix. That would be a separate patch.

So Hans -- same goes for your call which can be added after Andres' patch series.

> + * @FW_OPT_NO_WARN: Quiet, avoid printing warning messages.
> + * @FW_OPT_NOCACHE: Firmware caching will be disabled for this request.

Although we have good documentation about this on
Documentation/driver-api/firmware/firmware_cache.rst best to describe this
a bit more here.

Please use (modulo adjust lengths accordingly):

* @FW_OPT_NOCACHE: Disables firmware caching. Firmware caching is used to
cache the firmware upon suspend, so that upon resume
races against the firmware file lookup on storage is
avoided. Used for calls where the file may be too
big, or where the driver takes charge of its own firmware
caching mechanism.

Note that an example driver which does its own firmware caching mechanism is
iwlwifi, IIRC.

> + * @FW_OPT_NOFALLBACK: Disable the fallback path. Takes precedence over
> + * &FW_OPT_UEVENT and &FW_OPT_USERHELPER.

Please replace "fallback path" with "fallback mechanism" to be consistent
with the documentation.

Luis

> + */
> enum fw_opt {
> FW_OPT_UEVENT = (1U << 0),
> FW_OPT_NOWAIT = (1U << 1),
> --
> 2.14.1
>
>

--
Do not panic

2018-04-21 14:35:42

[permalink] [raw]

Subject: Re: [PATCH 5/9] firmware: add functions to load firmware without warnings v4

On Tue, Apr 17, 2018 at 11:33:03AM -0400, Andres Rodriguez wrote:
> @@ -755,10 +779,11 @@ static void firmware_request_work_func(struct work_struct *work)
> }
>
> /**
> - * firmware_request_nowait() - asynchronous version of firmware_request
> + * firmware_request_nowait2() - asynchronous version of firmware_request
> * @module: module requesting the firmware
> * @uevent: sends uevent to copy the firmware image if this flag
> * is non-zero else the firmware copy must be done manually.
> + * @warn: enable warnings
> * @name: name of firmware file
> * @device: device for which firmware is being loaded
> * @gfp: allocation flags
> @@ -778,8 +803,8 @@ static void firmware_request_work_func(struct work_struct *work)
> * - can't sleep at all if @gfp is GFP_ATOMIC.
> **/
> int
> -firmware_request_nowait(
> - struct module *module, bool uevent,
> +firmware_request_nowait2(
> + struct module *module, bool uevent, bool warn,
> const char *name, struct device *device, gfp_t gfp, void *context,
> void (*cont)(const struct firmware *fw, void *context))
> {
> @@ -799,7 +824,8 @@ firmware_request_nowait(
> fw_work->context = context;
> fw_work->cont = cont;
> fw_work->opt_flags = FW_OPT_NOWAIT |
> - (uevent ? FW_OPT_UEVENT : FW_OPT_USERHELPER);
> + (uevent ? FW_OPT_UEVENT : FW_OPT_USERHELPER) |
> + (warn ? 0 : FW_OPT_NO_WARN);
>
> if (!uevent && fw_cache_is_setup(device, name)) {
> kfree_const(fw_work->name);
> @@ -818,6 +844,24 @@ firmware_request_nowait(
> schedule_work(&fw_work->work);
> return 0;
> }
> +EXPORT_SYMBOL_GPL(firmware_request_nowait2);
> +
> +/**
> + * firmware_request_nowait() - compatibility version of firmware_request_nowait2
> + *
> + * This is equivalent to calling firmware_request_nowait2 with warnings enabled.
> + *
> + * Refer to firmware_request_nowait2 for further details.
> + **/
> +int
> +firmware_request_nowait(
> + struct module *module, bool uevent,
> + const char *name, struct device *device, gfp_t gfp, void *context,
> + void (*cont)(const struct firmware *fw, void *context))
> +{
> + return firmware_request_nowait2(module, uevent, true, name, device,
> + gfp, context, cont);
> +}
> EXPORT_SYMBOL(firmware_request_nowait);
>
> #ifdef CONFIG_PM_SLEEP

Ugh this is precisely the type of naming issue I predicted *years ago*
about the unflexibility of the naming scheme we used. Greg, since you had
sent us this rabbit hole, any name preference here? Please review what is
proposed and also suggest a scheme which you do prefer. I'm done with
the bikeshedding and just want to move on, but in a way that scales.

Luis

2018-04-21 14:54:26

[permalink] [raw]

Subject: Re: [PATCH 5/9] firmware: add functions to load firmware without warnings v4

On Sat, Apr 21, 2018 at 04:32:06PM +0200, Luis R. Rodriguez wrote:
> On Tue, Apr 17, 2018 at 11:33:03AM -0400, Andres Rodriguez wrote:
> > @@ -755,10 +779,11 @@ static void firmware_request_work_func(struct work_struct *work)
> > }
> >
> > /**
> > - * firmware_request_nowait() - asynchronous version of firmware_request
> > + * firmware_request_nowait2() - asynchronous version of firmware_request
> > * @module: module requesting the firmware
> > * @uevent: sends uevent to copy the firmware image if this flag
> > * is non-zero else the firmware copy must be done manually.
> > + * @warn: enable warnings
> > * @name: name of firmware file
> > * @device: device for which firmware is being loaded
> > * @gfp: allocation flags
> > @@ -778,8 +803,8 @@ static void firmware_request_work_func(struct work_struct *work)
> > * - can't sleep at all if @gfp is GFP_ATOMIC.
> > **/
> > int
> > -firmware_request_nowait(
> > - struct module *module, bool uevent,
> > +firmware_request_nowait2(
> > + struct module *module, bool uevent, bool warn,
> > const char *name, struct device *device, gfp_t gfp, void *context,
> > void (*cont)(const struct firmware *fw, void *context))
> > {
> > @@ -799,7 +824,8 @@ firmware_request_nowait(
> > fw_work->context = context;
> > fw_work->cont = cont;
> > fw_work->opt_flags = FW_OPT_NOWAIT |
> > - (uevent ? FW_OPT_UEVENT : FW_OPT_USERHELPER);
> > + (uevent ? FW_OPT_UEVENT : FW_OPT_USERHELPER) |
> > + (warn ? 0 : FW_OPT_NO_WARN);
> >
> > if (!uevent && fw_cache_is_setup(device, name)) {
> > kfree_const(fw_work->name);
> > @@ -818,6 +844,24 @@ firmware_request_nowait(
> > schedule_work(&fw_work->work);
> > return 0;
> > }
> > +EXPORT_SYMBOL_GPL(firmware_request_nowait2);
> > +
> > +/**
> > + * firmware_request_nowait() - compatibility version of firmware_request_nowait2
> > + *
> > + * This is equivalent to calling firmware_request_nowait2 with warnings enabled.
> > + *
> > + * Refer to firmware_request_nowait2 for further details.
> > + **/
> > +int
> > +firmware_request_nowait(
> > + struct module *module, bool uevent,
> > + const char *name, struct device *device, gfp_t gfp, void *context,
> > + void (*cont)(const struct firmware *fw, void *context))
> > +{
> > + return firmware_request_nowait2(module, uevent, true, name, device,
> > + gfp, context, cont);
> > +}
> > EXPORT_SYMBOL(firmware_request_nowait);
> >
> > #ifdef CONFIG_PM_SLEEP
>
> Ugh this is precisely the type of naming issue I predicted *years ago*
> about the unflexibility of the naming scheme we used. Greg, since you had
> sent us this rabbit hole, any name preference here? Please review what is
> proposed and also suggest a scheme which you do prefer. I'm done with
> the bikeshedding and just want to move on, but in a way that scales.

I'll side for now with Kalle's suggestion of having:

firmware_request_nowait_nowarn()

as nasty as it may seem. And this is just because we embarked on
the path to not have parameters passed to modify the calls site.

Luis

2018-04-21 15:14:28

by Kees Cook

[permalink] [raw]

Subject: Re: [PATCH 5/9] firmware: add functions to load firmware without warnings v4

On Sat, Apr 21, 2018 at 7:49 AM, Luis R. Rodriguez <[email protected]> wrote:
> On Sat, Apr 21, 2018 at 04:32:06PM +0200, Luis R. Rodriguez wrote:
>> On Tue, Apr 17, 2018 at 11:33:03AM -0400, Andres Rodriguez wrote:
>> > @@ -755,10 +779,11 @@ static void firmware_request_work_func(struct work_struct *work)
>> > }
>> >
>> > /**
>> > - * firmware_request_nowait() - asynchronous version of firmware_request
>> > + * firmware_request_nowait2() - asynchronous version of firmware_request
>> > * @module: module requesting the firmware
>> > * @uevent: sends uevent to copy the firmware image if this flag
>> > * is non-zero else the firmware copy must be done manually.
>> > + * @warn: enable warnings
>> > * @name: name of firmware file
>> > * @device: device for which firmware is being loaded
>> > * @gfp: allocation flags
>> > @@ -778,8 +803,8 @@ static void firmware_request_work_func(struct work_struct *work)
>> > * - can't sleep at all if @gfp is GFP_ATOMIC.
>> > **/
>> > int
>> > -firmware_request_nowait(
>> > - struct module *module, bool uevent,
>> > +firmware_request_nowait2(
>> > + struct module *module, bool uevent, bool warn,
>> > const char *name, struct device *device, gfp_t gfp, void *context,
>> > void (*cont)(const struct firmware *fw, void *context))
>> > {
>> > @@ -799,7 +824,8 @@ firmware_request_nowait(
>> > fw_work->context = context;
>> > fw_work->cont = cont;
>> > fw_work->opt_flags = FW_OPT_NOWAIT |
>> > - (uevent ? FW_OPT_UEVENT : FW_OPT_USERHELPER);
>> > + (uevent ? FW_OPT_UEVENT : FW_OPT_USERHELPER) |
>> > + (warn ? 0 : FW_OPT_NO_WARN);
>> >
>> > if (!uevent && fw_cache_is_setup(device, name)) {
>> > kfree_const(fw_work->name);
>> > @@ -818,6 +844,24 @@ firmware_request_nowait(
>> > schedule_work(&fw_work->work);
>> > return 0;
>> > }
>> > +EXPORT_SYMBOL_GPL(firmware_request_nowait2);
>> > +
>> > +/**
>> > + * firmware_request_nowait() - compatibility version of firmware_request_nowait2
>> > + *
>> > + * This is equivalent to calling firmware_request_nowait2 with warnings enabled.
>> > + *
>> > + * Refer to firmware_request_nowait2 for further details.
>> > + **/
>> > +int
>> > +firmware_request_nowait(
>> > + struct module *module, bool uevent,
>> > + const char *name, struct device *device, gfp_t gfp, void *context,
>> > + void (*cont)(const struct firmware *fw, void *context))
>> > +{
>> > + return firmware_request_nowait2(module, uevent, true, name, device,
>> > + gfp, context, cont);
>> > +}
>> > EXPORT_SYMBOL(firmware_request_nowait);
>> >
>> > #ifdef CONFIG_PM_SLEEP
>>
>> Ugh this is precisely the type of naming issue I predicted *years ago*
>> about the unflexibility of the naming scheme we used. Greg, since you had
>> sent us this rabbit hole, any name preference here? Please review what is
>> proposed and also suggest a scheme which you do prefer. I'm done with
>> the bikeshedding and just want to move on, but in a way that scales.
>
> I'll side for now with Kalle's suggestion of having:
>
> firmware_request_nowait_nowarn()
>
> as nasty as it may seem. And this is just because we embarked on
> the path to not have parameters passed to modify the calls site.

What was the objection to using parameters for this? i.e. something
like the gfp flags, but have a behavior flag FW_RQ_NOWAIT,
FW_RQ_NOWARN, etc?

-Kees

--
Kees Cook
Pixel Security

2018-04-21 15:36:53

by Linus Torvalds

[permalink] [raw]

Subject: Re: [PATCH 5/9] firmware: add functions to load firmware without warnings v4

On Sat, Apr 21, 2018 at 8:11 AM, Kees Cook <[email protected]> wrote:
>
> What was the objection to using parameters for this? i.e. something
> like the gfp flags, but have a behavior flag FW_RQ_NOWAIT,
> FW_RQ_NOWARN, etc?

The objection was that the patches that I think Luis refers to

(a) passed in a union of random arguments

(b) changed all the users, even the ones that didn't want to be changed.

they were nasty and illegible and pointless.

Using some single flag field for an extended function, and leaving the
existing functions alone so that you don't have to convert existing
users - that would have been fine. That's not what was tried and
rejected.

Linus

2018-04-21 17:38:18

[permalink] [raw]

Subject: Re: [PATCH 5/9] firmware: add functions to load firmware without warnings v4

On Sat, Apr 21, 2018 at 08:32:00AM -0700, Linus Torvalds wrote:
> On Sat, Apr 21, 2018 at 8:11 AM, Kees Cook <[email protected]> wrote:
> >
> > What was the objection to using parameters for this? i.e. something
> > like the gfp flags, but have a behavior flag FW_RQ_NOWAIT,
> > FW_RQ_NOWARN, etc?
>
> The objection was that the patches that I think Luis refers to

There were different topics of discussion, people can conflate the topics
discussed easily as all these topics were discussed around the same time so
best to recap and split them up:

a) bikeshedding on the name: I suggested long ago we've already passed usage
of the fw API to things which are *not firmware*, so suggested we use a new
naming scheme, the last one I recommended was a driver_data_*() prefix.
Greg last suggested a firmware_*() prefix -- and to rename *all* existing
callers with this new prefix as well long term... I am done with bikeshedding
and frankly don't care anymore. Whatever you folks decide, I just want to
move on with life, so I am moving along with the firmware_*() prefix.

b) evolving the API: data driven Vs functional -- where to draw the fine line
-- this is the topic we are discussing now again --

c) firmware signing: we've decided against it for now and folks who need it
can open code it, mainly due to most hw supporting FW signing already

> (a) passed in a union of random arguments

It split the API into two main routine calls, a sync and an async call,
and also accepted a *structure of parameters* which describe the request
requirements [0].

It turned out this approach was *too data driven*, and Greg advised against
it, asking for a new call *per new functionality*, as is typically done...

[0] https://lkml.kernel.org/r/[email protected]

> (b) changed all the users, even the ones that didn't want to be changed.

That *never happened* actually. The SmPL grammar I provided long ago was for
those who *did* want to change the new API if they had a new *feature* they
wanted to take advantage of. The patches you may have seen were *examples* of
*two drivers* which were converted over just an example with the grammar.

On the last iteration of my series I skipped adding the SmPL helpers, and
only converted *one* driver for *new functionality*.

In the last driver data patch series I only converted iwlwifi as I was changing
it to use the new functionality to replace its own internal recursion set of
calls with an internal deterministic solution which lets drivers call for a
range of API files and lets it use the first one in a range it finds [1],
with this diff stat:

drivers/net/wireless/intel/iwlwifi/iwl-drv.c | 91 ++++++++++------------------
1 file changed, 31 insertions(+), 60 deletions(-)

[1] https://lkml.kernel.org/r/[email protected]

> they were nasty and illegible and pointless.

Clearly request_firmware_nowait2() is *not* much better... right? So it illustrates
the problem I was hinting which we'd eventually cross...

> Using some single flag field for an extended function, and leaving the
> existing functions alone so that you don't have to convert existing
> users - that would have been fine. That's not what was tried and
> rejected.

Actually it was tried, however the divide was perhaps *too broad* and split
all possible *new* functionality into two calls, a sync and a async call.

A flag based mechanism *is* reasonable to me given I have been an advocate
of such type of mechanism for a long while. It however is against what
Greg requested -- to have a new call *per functionality*.

So feel free to advise... I really just want us to move on.

Luis

2018-04-22 20:27:38

[permalink] [raw]

Subject: Re: [PATCH 5/9] firmware: add functions to load firmware without warnings v4

On Sat, Apr 21, 2018 at 07:36:50PM +0200, Luis R. Rodriguez wrote:
> On Sat, Apr 21, 2018 at 08:32:00AM -0700, Linus Torvalds wrote:
> > they were nasty and illegible and pointless.
>
> Clearly request_firmware_nowait2() is *not* much better... right? So it illustrates
> the problem I was hinting which we'd eventually cross...
>
> > Using some single flag field for an extended function, and leaving the
> > existing functions alone so that you don't have to convert existing
> > users - that would have been fine. That's not what was tried and
> > rejected.
>
> Actually it was tried, however the divide was perhaps *too broad* and split
> all possible *new* functionality into two calls, a sync and a async call.
>
> A flag based mechanism *is* reasonable to me given I have been an advocate
> of such type of mechanism for a long while. It however is against what
> Greg requested -- to have a new call *per functionality*.
>
> So feel free to advise... I really just want us to move on.

Andres,

Since we haven't heard back, and I don't want to leave you hanging here is what
I recommend:

Re-submit and ignore the new async call for now. Leave that or another series
later. Note that Hans also has another series which we want to merge soon too,
so I expect we can address this async call after Hans's work.

What I recommend for advancing the API to support future async calls is
first we make it clear the current flags are private, then see if we can
stuff them into struct fw_priv, and pass that data structure around internally
where possible instead of using really long set of arguments on tons of
internal functions. That's at least one commit alone.

Once that is done I'd add public API flags which reflect the existing
custom use cases, the first flag would be the warn (or quiet) flag for now. We
can then pass these public flags around internally to modify behaviour.
That may be another commit.

Instead of doing only two calls (one async and one syc) as I had done in prior
submissions, we'd continue the ongoing practice of a new call per functionality
as Greg has suggested, however the flags would enable to *slightly* modify
behaviour. So you can add a new flexible async call which accepts the public
flags argument.

So new functionality per API but slight modifications are expressed via the
new public flags.

If you're up to try all all these changes please feel free to do so, I just
expect more possible bikeshedding on it so don't expect this to go in right
away.

Luis

2018-04-23 13:56:22